Bug 9209 - Parse of invalid SMB2 create blob can cause smbd crash.
Parse of invalid SMB2 create blob can cause smbd crash.
Status: RESOLVED FIXED
Product: Samba 3.6
Classification: Unclassified
Component: SMB2
unspecified
All All
: P5 regression
: ---
Assigned To: Karolin Seeger
Samba QA Contact
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-09-25 00:22 UTC by Jeremy Allison
Modified: 2012-10-31 08:54 UTC (History)
3 users (show)

See Also:


Attachments
git-am fix for 4.0.0rc2 and also 3.6.next. (4.41 KB, patch)
2012-09-28 17:20 UTC, Jeremy Allison
metze: review+
vl: review+
Details
Test for v4-0-test (4.48 KB, patch)
2012-10-10 07:07 UTC, Stefan Metzmacher
jra: review+
Details
Patch for v4-0-test and v3-6-test (1.09 KB, patch)
2012-10-27 09:05 UTC, Stefan Metzmacher
jra: review+
obnox: review+
vl: review+
metze: review? (asn)
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Jeremy Allison 2012-09-25 00:22:02 UTC
Found by Codenomicon at the SNIA plugfest. Patches for 4.0.0 and 3.6.next to follow.

Jeremy.
Comment 1 Jeremy Allison 2012-09-28 17:20:38 UTC
Created attachment 7956 [details]
git-am fix for 4.0.0rc2 and also 3.6.next.
Comment 2 Jeremy Allison 2012-09-28 17:53:24 UTC
Comment on attachment 7956 [details]
git-am fix for 4.0.0rc2 and also 3.6.next.

Patch also applies cleanly to 3.6.next.
Comment 3 Jeremy Allison 2012-09-28 20:39:28 UTC
Re-assigning to Karolin for inclusion in 3.6.next and 4.0.0rc2.
Jeremy.
Comment 4 Stefan Metzmacher 2012-09-29 08:17:56 UTC
Comment on attachment 7956 [details]
git-am fix for 4.0.0rc2 and also 3.6.next.

Looks good.

We may want to backport
e6c600aa2c751e694917322378417816c3e58eb6 (s4:torture/smb2: improve the smb2.create.blob test)
too
Comment 5 Karolin Seeger 2012-10-01 07:11:36 UTC
Pushed to autobuild-v4-0-test and v3-6-test.
Re-assigning to Jeremy because of Metze's backport comment.

Thanks!
Comment 6 Stefan Metzmacher 2012-10-10 07:07:40 UTC
Created attachment 8036 [details]
Test for v4-0-test
Comment 7 Jeremy Allison 2012-10-11 19:29:11 UTC
Comment on attachment 8036 [details]
Test for v4-0-test

LGTM.
Comment 8 Jeremy Allison 2012-10-11 19:29:42 UTC
Re-assigning to Karolin to push Metze's 4.0.0rc3 test.
Comment 9 Karolin Seeger 2012-10-12 07:31:16 UTC
(In reply to comment #8)
> Re-assigning to Karolin to push Metze's 4.0.0rc3 test.

Pushed to autobuild-v4-0-test.
Closing out bug report.

Thanks!
Comment 10 Stefan Metzmacher 2012-10-27 09:05:03 UTC
Created attachment 8110 [details]
Patch for v4-0-test and v3-6-test

This fixes a problem in https://bugzilla.samba.org/attachment.cgi?id=7956
where we send uninitialized padding.

I think this is needed for 3.6.9
Comment 11 Stefan Metzmacher 2012-10-27 09:07:10 UTC
Karolin, this is needed for 3.6.9 otherwise we introduce a regression.
Comment 12 Stefan Metzmacher 2012-10-27 09:11:57 UTC
(In reply to comment #10)
> Created attachment 8110 [details]
> Patch for v4-0-test and v3-6-test
> 
> This fixes a problem in https://bugzilla.samba.org/attachment.cgi?id=7956
> where we send uninitialized padding.
> 
> I think this is needed for 3.6.9

A bit more information on the patch:

-       blob_offset = 0x14 + tag_length;
+       blob_offset = 0x10 + tag_length;
        blob_pad = smb2_create_blob_padding(blob_offset, 8);

The resulting blob_offset will be the same, as we later have this:

        if (blob_pad > 0) {
                memset(buffer->data+ofs+blob_offset, 0, blob_pad);
                blob_offset += blob_pad;
        }

The important part is that blob_pad changes from 0 to 4
and we initialize the 4 byte padding (for the typical case where
tag_length is 4).
Comment 13 Andreas Schneider 2012-10-27 16:51:58 UTC
Do you plan to backport the torture test for 3.6 too?
Comment 14 Stefan Metzmacher 2012-10-28 09:25:57 UTC
(In reply to comment #13)
> Do you plan to backport the torture test for 3.6 too?

I don't think that is needed
Comment 15 Karolin Seeger 2012-10-28 18:44:11 UTC
(In reply to comment #11)
> Karolin, this is needed for 3.6.9 otherwise we introduce a regression.

Okay, thanks for the heads-up.

Samba 3.6.9 is scheduled for tomorrow, please review this patch asap.
Additionally, I would like to propose to delay the release to avoid including last minute patches and allow proper testing/review.

Comments welcome.

Thanks!
Comment 16 Michael Adam 2012-10-28 23:00:20 UTC
Comment on attachment 8110 [details]
Patch for v4-0-test and v3-6-test

ack
Comment 17 Michael Adam 2012-10-28 23:20:46 UTC
Assigning to Karolin for inclusion into v3-6-test and v4-0-test
Comment 18 Andreas Schneider 2012-10-29 07:36:47 UTC
It looks like the test case is simple and can be applied without problems to 3.6. I would like to have the test which verifies that it is fixed in 3.6 too so QA can verify it.
Comment 19 Stefan Metzmacher 2012-10-29 07:43:08 UTC
(In reply to comment #18)
> It looks like the test case is simple and can be applied without problems to
> 3.6. I would like to have the test which verifies that it is fixed in 3.6 too
> so QA can verify it.

If you propose a patch (or if the v4-0-test one applies) I'm happy to ack it...
Comment 20 Karolin Seeger 2012-10-29 08:04:44 UTC
(In reply to comment #15)
> (In reply to comment #11)
> > Karolin, this is needed for 3.6.9 otherwise we introduce a regression.
> 
> Okay, thanks for the heads-up.
> 
> Samba 3.6.9 is scheduled for tomorrow, please review this patch asap.
> Additionally, I would like to propose to delay the release to avoid including
> last minute patches and allow proper testing/review.
> 
> Comments welcome.
> 
> Thanks!

Talked to Metze and it seems to be an obvious fix which is needed for 3.6.9.
Going ahead with the release.
Comment 21 Karolin Seeger 2012-10-29 08:05:50 UTC
Pushed to v3-6-test and autobuild-v4-0-test.
Will be included in 3.6.9.

Re-assigning to Andreas as he seems to work on a test.
Comment 22 Andreas Schneider 2012-10-29 08:10:48 UTC
The "Test for v4-0-test" patch from comment #6 applies cleanly to v3.6-test.
Comment 23 Stefan Metzmacher 2012-10-29 08:26:26 UTC
(In reply to comment #22)
> The "Test for v4-0-test" patch from comment #6 applies cleanly to v3.6-test.

Ok, then I'm fine if it gets backported to v3-6-test.
Comment 24 Jeremy Allison 2012-10-29 16:52:11 UTC
Comment on attachment 8110 [details]
Patch for v4-0-test and v3-6-test

From the "better late than never" dept. +1 from me (it was over my weekend :-).
Jeremy.
Comment 25 Stefan Metzmacher 2012-10-31 01:42:36 UTC
Karolin, please also pick the test/torture patch to v3-6-test
Comment 26 Karolin Seeger 2012-10-31 08:54:56 UTC
(In reply to comment #25)
> Karolin, please also pick the test/torture patch to v3-6-test
(In reply to comment #25)
> Karolin, please also pick the test/torture patch to v3-6-test

Pushed.
Closing out bug report.

Thanks!