streams_xattr_pwrite() in vfs_streams_xattr can write uninitialized memory to alternate data streams if you issue a write request that creates a hole in the file because we don't zero out the new buffer. For example, first write a small stream, then issue a write to a high offset, then look at xattr contents. https://gitlab.com/samba-team/samba/-/blob/master/source3/modules/vfs_streams_xattr.c?ref_type=heads#L1051 as opposed to https://gitlab.com/samba-team/samba/-/blob/master/source3/modules/vfs_streams_xattr.c?ref_type=heads#L1325 This allows clients to basically write uninitialized memory to the xattr, then read it back, (rinse and repeat). The following appears to resolve the issue: ``` diff --git a/source3/modules/vfs_streams_xattr.c b/source3/modules/vfs_streams_xattr.c index ac01cc46043..d2d3562d368 100644 --- a/source3/modules/vfs_streams_xattr.c +++ b/source3/modules/vfs_streams_xattr.c @@ -1047,18 +1047,18 @@ static ssize_t streams_xattr_pwrite(vfs_handle_struct *handle, if ((offset + n) > ea.value.length-1) { uint8_t *tmp; + size_t new_sz = offset + n + 1; - tmp = talloc_realloc(talloc_tos(), ea.value.data, uint8_t, - offset + n + 1); + tmp = talloc_realloc(talloc_tos(), ea.value.data, uint8_t, new_sz); if (tmp == NULL) { TALLOC_FREE(ea.value.data); errno = ENOMEM; return -1; } + memset(tmp + ea.value.length, 0, new_sz - ea.value.length); ea.value.data = tmp; - ea.value.length = offset + n + 1; - ea.value.data[offset+n] = 0; + ea.value.length = new_sz; } ```
My guess at CVSS 3.1 is 4.3 or 6.5: AV:N/AC:L/PR:L/UI:N/S:U/C:L/I:N/A:N via https://nvd.nist.gov/vuln-metrics/cvss/v3-calculator depending on whether I set "confidentiality impact" to low or high, assuming it needs an authenticated user and low complexity.
Where are we with the discussion if this is worth a CVE? I'm currently working on streams_xattr and I would like to be able to propose code in that area.
(In reply to Douglas Bagnall from comment #1) To me it's C:L so the score is 4.3. That would iirc mean we don't do a full security release and instead just release this as a bugfix with a CVE assigned.
(In reply to Ralph Böhme from comment #3) > To me it's C:L so the score is 4.3. That would iirc mean we don't do > a full security release and instead just release this as a bugfix > with a CVE assigned. Yeah, depending also on how we feel about it, but so far nobody seems too bothered. I've asked for a CVE. Andrew, can you git-format-patch the fix? I think we are absolved from writing the full security advisory if we don't do a security release.
Created attachment 18696 [details] v4.23 fix
Created attachment 18697 [details] v4.23 test
I wrote this initial patchset against v4.23-stable. Let me know if you want changes to either the patch or the test and I can rework as needed.
Comment on attachment 18697 [details] v4.23 test Haven't tested it myself, but the test looks good just from looking at it.
Created attachment 18713 [details] drafdt advisory I wrote > I think we are absolved from writing the full security advisory if we don't do a security release. Actually on second thoughts we might as well, since we need to explain what is going on anyway. I have had a bit of a go. Andrew or Volker, can you expand it a little bit and/or make it make sense?
One problem: I've already published a different fix as part of MR4212. This bug was the reason why I created talloc_realloc_zero(), nobody seemed too worried.
(In reply to Volker Lendecke from comment #10) I did wonder if that touched on this. Is it backportable? I'm don't know what the right thing to do is.
(In reply to Douglas Bagnall from comment #11) > Is it backportable? > > I'm don't know what the right thing to do is. I'm thinking the simple fix should become part of the October 1 security release that we are already doing, then !4212 can go on top in master.
Created attachment 18736 [details] draft patch v2 The test needed a couple of tweaks to compile (also some trailing whitespace removed): --- c/source4/torture/vfs/streams_xattr.c +++ w/source4/torture/vfs/streams_xattr.c @@ -39,7 +39,7 @@ static bool get_stream_handle(struct torture_context *tctx, const char *sname, struct smb2_handle *hdl_in) { - bool ret = true, ok; + bool ret = true; NTSTATUS status; struct smb2_handle fhandle = {{0}}; struct smb2_handle dhandle = {{0}}; @@ -88,7 +88,7 @@ static bool read_stream(struct torture_context *tctx, status = smb2_read(tree, mem_ctx, &r); torture_assert_ntstatus_ok_goto(tctx, status, ret, done, "stream read\n"); - *data_out = r.out.data.data; + *data_out = (char *)r.out.data.data; *data_out_sz = r.out.data.length; but I am still having trouble finding a way to make it run, so that I can check it fails. Are the source4/torture/vfs/ tests run at all?
Created attachment 18737 [details] advisory v2
Created attachment 18738 [details] draft patch v3 patched the test into source3/selftest/tests.py, though it isn't failing without the fix.
Created attachment 18739 [details] patch v4
Created attachment 18740 [details] patch v4
(In reply to Douglas Bagnall from comment #15) > it isn't failing without the fix. This is still the case.
Created attachment 18741 [details] patch v5 > it isn't failing without the fix. because I wasn't running it in a share with streams_xattr. NOW it fails without the fix.
Created attachment 18745 [details] patch for 4.23
Created attachment 18746 [details] patch for 4.22
Created attachment 18747 [details] patch for 4.21
Created attachment 18753 [details] advisory v3 Advisory v3 with $VERSIONS expanded.
This bug was referenced in samba v4-21-stable (Release samba-4.21.9): ad80099b697d280bc03e47b9b8ab33615b0db262 52969774d136644360f998b329736c19e43f8140
This bug was referenced in samba v4-22-stable (Release samba-4.22.5): 44d71234dfffa52ad2579924813b67c9d8a37822 06bc23b5977f564fef31285a92ec28dc80b68edb
This bug was referenced in samba v4-23-stable (Release samba-4.23.2): 25cad91bcf14bbccb53077ceb8b5be2d1ef9ebc3 26b3ff3752eb90d21c2c1db51c16ba8a15fd7429
Removing vendor CC (so that any public comments don't need to be broadcast so widely) and opening these bugs to the public. If you wish to continue to be informed about any changes here please CC individually.
This bug was referenced in samba v4-23-test: 25cad91bcf14bbccb53077ceb8b5be2d1ef9ebc3 26b3ff3752eb90d21c2c1db51c16ba8a15fd7429
This bug was referenced in samba v4-22-test: 44d71234dfffa52ad2579924813b67c9d8a37822 06bc23b5977f564fef31285a92ec28dc80b68edb
This bug was referenced in samba v4-21-test: ad80099b697d280bc03e47b9b8ab33615b0db262 52969774d136644360f998b329736c19e43f8140
This bug was referenced in samba master: 59158cc3b74835193e0b058561206a3198adc3fe 1e899521e821f2ee4cbb93f6a3befd37f5ba0403