BTW, I did notr find this in my private fuzzing because I didn't have --address-sanitizer in my configure line. I had removed that because it plays havoc with valgrind and gdb and did not seem to be finding new bugs.

The lesson is, have two fuzzing trees -- one for fuzzing with asan enabled, and one without for debugging.

(In reply to Douglas Bagnall from comment #0)

> If m is an odd number, then it will never get to <= 0 (because size_t)

Indeed m starts off at 1.

Created attachment 15736 [details]
[patch] this might be all we need

Comment on attachment 15736 [details]
[patch] this might be all we need

Oh, good catch ! Do you think this is a CVE-level problem ? m is read from the incoming packet, so it can be set to 1 by the attacker.

The following code:

 213                 if (n >= sizeof(name->name)) {
 214                         return 0;
 215                 }
 216                 name->name[n++] = (c1<<4) | c2;

will break us out of the loop even if m wraps through zero with the -2.

(In reply to Jeremy Allison from comment #4)

So I'm guessing we can only overwrite by 1 byte.

(In reply to Jeremy Allison from comment #5)

I think we can't overwrite, but we can over-read by 1-15 bytes?

It will return 0 before writing into name->name[16]

(In reply to Douglas Bagnall from comment #6)

Ah, but we can't overread, because we have checked offset + m against length.

So no security issue?

Created attachment 15737 [details]
raw test patch.

I don't think it's a security issue - just a one-byte overread.

What do you think of the following patch ?

Does it make things clearer ?

I'd love to get rid of the horrible 'int' variables in this code, but it's old and fragile enough I'm loathe to mess with it.

What this

if ((m & 0xC0) ||

in the existing code is *really* saying is

if ((m >= 32) ||

It would be worth putting it that way while we're there:

if (m < 2 || m > 30) {

(In reply to Douglas Bagnall from comment #9)

> in the existing code is *really* saying is
>
> if ((m >= 32) ||

Actually it appears to be 'if (m > 32)', as the
value of m for a standard name lookup seems to
be always 32 (from additional debugs I added).

Yep, it's not null terminated - the 'name type' is in the last byte.

Here's how we post-process it once it's been read into name->name:

from source3/include/smb.h:

660 #define MAX_NETBIOSNAME_LEN 16
661 /* DOS character, NetBIOS namestring. Type used on the wire. */
662 typedef char nstring[MAX_NETBIOSNAME_LEN];
663 /* Unix character, NetBIOS namestring. Type used to manipulate name in nmbd.
664 typedef char unstring[MAX_NETBIOSNAME_LEN*4];
665 
666 /* A netbios name structure. */
667 struct nmb_name {
668         nstring      name;
669         char         scope[64];
670         unsigned int name_type;
671 };

from source3/libsmb/nmblib.c:

 230         /*
 231          * RFC1002: For a valid NetBIOS name, exiting from the above,
 232          * n *must* be MAX_NETBIOSNAME_LEN (16).
 233          */
 234         if (n != MAX_NETBIOSNAME_LEN) {
 235                 return 0;
 236         }
 237 
 238         /* parse out the name type, its always
 239          * in the 16th byte of the name */
 240         name->name_type = ((unsigned char)name->name[15]) & 0xff;
 241 
 242         /* remove trailing spaces */
 243         name->name[15] = 0;
 244         n = 14;
 245         while (n && name->name[n]==' ')
 246                 name->name[n--] = 0;

So the max length is 32, not 30.

Yep, it's not null terminated - the 'name type' is in the last byte.

Here's how we post-process it once it's been read into name->name:

from source3/include/smb.h:

660 #define MAX_NETBIOSNAME_LEN 16
661 /* DOS character, NetBIOS namestring. Type used on the wire. */
662 typedef char nstring[MAX_NETBIOSNAME_LEN];
663 /* Unix character, NetBIOS namestring. Type used to manipulate name in nmbd.
664 typedef char unstring[MAX_NETBIOSNAME_LEN*4];
665 
666 /* A netbios name structure. */
667 struct nmb_name {
668         nstring      name;
669         char         scope[64];
670         unsigned int name_type;
671 };

from source3/libsmb/nmblib.c:

 230         /*
 231          * RFC1002: For a valid NetBIOS name, exiting from the above,
 232          * n *must* be MAX_NETBIOSNAME_LEN (16).
 233          */
 234         if (n != MAX_NETBIOSNAME_LEN) {
 235                 return 0;
 236         }
 237 
 238         /* parse out the name type, its always
 239          * in the 16th byte of the name */
 240         name->name_type = ((unsigned char)name->name[15]) & 0xff;
 241 
 242         /* remove trailing spaces */
 243         name->name[15] = 0;
 244         n = 14;
 245         while (n && name->name[n]==' ')
 246                 name->name[n--] = 0;

So the max length is 32, not 30.

(In reply to Jeremy Allison from comment #10)
Ah right.

As I noted on the merge request, because of the 

 234         if (n != MAX_NETBIOSNAME_LEN) {
 235                 return 0;
 236         }

I see no reason we should accept anything but 32. It's going to fail anyway.

Yep, I think your fix on the merge request is the right one.

I'll try and make the change over the weekend and re-push to the CI (unless you do it first :-).

Created attachment 15749 [details]
git-am fix that went into master

Applies cleanly to 4.12.rc1, 4.11.next, 4.10.next, 4.9.next.

Re-assigning to Karolin for inclusion in 4.12rcNext, 4.11.next, 4.10.next, 4.9.next.

Pushed to autobuild-v4-{12,11,10}-test.

(In reply to Karolin Seeger from comment #17)
Pushed to all branches.
Closing out bug report.

Thanks!

Removing embargo, which was left on by mistake after it was closed.

We decided this was not a security issue.

It was fixed with ad236bb7590e423b4c69fe6028f2f3495977f48b