Created attachment 16581 [details]
the read crash

Created attachment 16582 [details]
the write crash

(In reply to Douglas Bagnall from comment #0)

The valgrind line numbers and file names were off (master has changed). Here are up-to-date snippets:

==3784219== Invalid read of size 1
==3784219==    at 0x483EF46: strlen (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==3784219==    by 0x486D409: talloc_string_sub2 (substitute.c:248)
==3784219==    by 0x1098C8: LLVMFuzzerTestOneInput (fuzz_talloc_string_sub2.c:63)
==3784219==    by 0x1092F9: main (afl-fuzz-main.c:46)
==3784219==  Address 0x74f4842 is 30 bytes before a block of size 528 in arena "client"


==3785419== Invalid write of size 8
==3785419==    at 0x4842A9B: memmove (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==3785419==    by 0x486D445: talloc_string_sub2 (substitute.c:250)
==3785419==    by 0x1098C8: LLVMFuzzerTestOneInput (fuzz_talloc_string_sub2.c:63)
==3785419==    by 0x1092F9: main (afl-fuzz-main.c:46)
==3785419==  Address 0x74f4818 is 568 bytes inside a block of size 572 alloc'd
==3785419==    at 0x483B7F3: malloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==3785419==    by 0x48D0365: __talloc_with_prefix (talloc.c:783)
==3785419==    by 0x48D04FF: __talloc (talloc.c:825)
==3785419==    by 0x48D3FDC: __talloc_strlendup (talloc.c:2454)
==3785419==    by 0x48D4096: talloc_strdup (talloc.c:2470)
==3785419==    by 0x486D142: talloc_string_sub2 (substitute.c:185)
==3785419==    by 0x1098C8: LLVMFuzzerTestOneInput (fuzz_talloc_string_sub2.c:63)
==3785419==    by 0x1092F9: main (afl-fuzz-main.c:46)


In substitute.c, 248 is this memmove, and 250 is the memcpy, and the problem in either case is that p is wrong, due to the strstr_m error:


		if (li != lp) {
			memmove(p+li,p+lp,strlen(p+lp)+1);
		}
		memcpy(p, in, li);
		s = p + li;
		ls += ld;

Hi Douglas, can you give me an example of a "bad" input to strstr_m() so I can try and follow what is going wrong here. Currently I don't understand the bug :-(.

Created attachment 16583 [details]
proof of concept not reliant on fuzzers

Not quite what you asked for Jeremy, but a step in that direction. Here is a stand alone proof of concept.

$ bin/default/lib/util/crash_talloc_string_sub2
Aborted

Created attachment 16584 [details]
proof of concept

THIS is the proof-of-concept patch that will actually apply on master.

Oh that's perfect Douglas, thanks ! I can now walk through this with a debugger and figure out what's going wrong (hopefully). Thanks *so much* for doing this.

In strstr_m, push_ucs2_talloc(mem_ctx, &find_w, findstr, &converted_size) is putting a 0 uint16_t at around position 146? 

and so it looks like that matches the beginning of src_w.

(In reply to Douglas Bagnall from comment #8)
because codepoint F002 is in a private use area?

I will stop for now.

Created attachment 16585 [details]
the crash test reduced to short mostly printable strings

The new attachment is this:

#include "includes.h"

const char string[] =  "++++++\xD8\xBB";
const char pattern[] = "++++\xF0\x80\x80\x80+++++";
const char replace[] = "......................";

int main(int argc, char *argv[])
{
	char *t = NULL;
	printf("%s\n %zu\n", string, strlen(string));
	printf("%s\n %zu\n", pattern, strlen(pattern));

	t = talloc_string_sub(NULL, string, pattern, replace);

	printf("%s\n %zu\n", t, strlen(t));
	talloc_free(t);
	return 0;
}



which produces a lot of output under valgrind, and this when run normally:


++++++ػ
 8
++++����+++++
 13
......................
 22

(sometimes finishing earlier with "munmap_chunk(): invalid pointer\n Aborted").


My theory is still that the high bytes in *pattern* look like a terminator, so *pattern* looks like it is "++++", matching the start of *string*. Then it loses track of which length it is using.

If *string* contains no high bytes we don't seem to go down this road.

So there are sort of 2 bugs. One is that we do this replacement at all. The other is that we do it so badly.

I haven't found callers where the pattern is not a known constant string (typically something like "%M"), in which case there is no security concern, or even any need to backport. But I haven't looked exhaustively.

Ha, yes, the sequence "\xF0\x80\x80\x80" is read by utf8_pull() in the smb_iconv code as the 4-byte encoding of '\0'.

As you can see, it skips past the high bits check here, then as there are no low bits it encodes zero:

		if ((c[0] & 0xf8) == 0xf0) {
			unsigned int codepoint;
			if (in_left < 4 ||
			    (c[1] & 0xc0) != 0x80 ||
			    (c[2] & 0xc0) != 0x80 ||
			    (c[3] & 0xc0) != 0x80) {
				errno = EILSEQ;
				goto error;
			}
			codepoint =
				(c[3]&0x3f) |
				((c[2]&0x3f)<<6) |
				((c[1]&0x3f)<<12) |
				((c[0]&0x7)<<18);


(that's lib/util/charset/iconv.c line 858-872).


*Presumably* real iconv rejects over-length encodings like this, and users of that will be fine. I believe the spec and battle-worn wisdom agree on that behaviour.

The question is: does anyone really use our iconv? If they do, we really need to fix this, as there will be other routes to the same bug. If not, well, we might as well fix it to get compatible behaviour.

Created attachment 16586 [details]
The fix

With this patch, we get the following. Note that not only does it not upset valgrind, it also produces the right answer ("++++����+++++" does not occur in "++++++ػ", so there is no substitution).


$ valgrind ./bin/default/lib/util/crash_talloc_string_sub2
==29596== Memcheck, a memory error detector
==29596== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==29596== Using Valgrind-3.14.0 and LibVEX; rerun with -h for copyright info
==29596== Command: ./bin/default/lib/util/crash_talloc_string_sub2
==29596== 
++++++ػ
 8
++++����+++++
 13
++++++ػ
 8
==29596== 
==29596== HEAP SUMMARY:
==29596==     in use at exit: 1,106 bytes in 6 blocks
==29596==   total heap usage: 1,238 allocs, 1,232 frees, 173,179 bytes allocated
==29596== 
==29596== LEAK SUMMARY:
==29596==    definitely lost: 0 bytes in 0 blocks
==29596==    indirectly lost: 0 bytes in 0 blocks
==29596==      possibly lost: 1,106 bytes in 6 blocks
==29596==    still reachable: 0 bytes in 0 blocks
==29596==         suppressed: 0 bytes in 0 blocks
==29596== Rerun with --leak-check=full to see details of leaked memory
==29596== 
==29596== For counts of detected and suppressed errors, rerun with: -v
==29596== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

(In reply to Douglas Bagnall from comment #13)

Douglas, this is *amazingly* great work - thanks ! I don't think anyone uses our iconv in preference to the system iconv, so I'm guessing this isn't a security issue.

But thanks a million for the fix !

(In reply to Jeremy Allison from comment #14)
> I don't think anyone uses our iconv in preference to the system iconv, 
> so I'm guessing this isn't a security issue.

Yeah, I wonder about embedded things, but they wouldn't get the patches anyway.

Should we just get rid of the internal iconv?

I will add a test or two.

Björn, since you just offered an informed opinion on the list about --without-iconv, I am wondering how many people you think might be a) affected by this, and b) able to patch?

(I'm assuming --without-iconv means using our local mini-iconv, where the bug lies).

Ping. Can we get this fix into master please ?

(In reply to Douglas Bagnall from comment #16)
the problem with some iconv implementations is that they do not have the charset names that Samba likes to make use of. On AIX for example you have these charsets available in iconv:

ASCII-GR BIG5-HKSCS CNS11643.1986-1 CNS11643.1986-2 GB18030 GBK IBM-037 IBM-1006 IBM-1046 IBM-1124 IBM-1129 IBM-1251 IBM-1252 IBM-1363 IBM-278 IBM-500 IBM-850 IBM-850 IBM-850-GL IBM-850-GR IBM-856 IBM-921 IBM-922 IBM-932 IBM-943 IBM-eucCN IBM-eucJP IBM-eucKR IBM-eucTW IBM-sbdTW IBM-udcJP IBM-udcTW ISCII.1991 ISO-2022-JP ISO-8859-1 ISO8859-1 ISO8859-1 ISO8859-1-GL ISO8859-1-GL ISO8859-1-GR ISO8859-1-GR ISO8859-15 ISO8859-15 ISO8859-15-GL ISO8859-15-GR ISO8859-2 ISO8859-2-GL ISO8859-2-GR ISO8859-3 ISO8859-3-GL ISO8859-3-GR ISO8859-4 ISO8859-4-GL ISO8859-4-GR ISO8859-5 ISO8859-5-GL ISO8859-5-GR ISO8859-6 ISO8859-6-GL ISO8859-6-GR ISO8859-7 ISO8859-7-GL ISO8859-7-GR ISO8859-8 ISO8859-8-GL ISO8859-8-GR ISO8859-9 ISO8859-9-GL ISO8859-9-GR JISX0201.1976-0 JISX0208.1983-0 KSC5601.1987-0 KSC5601.1987-1 TIS-620 UCS-2 UNICODE-2 UTF-16 UTF-16le UTF-32 UTF-8 big5 ct fold7 fold8 uucode

No plain ASCII, no UCS-2LE for example, even though "UCS-2", which is available, is the same as UCS-2LE on AIX and on Linux' iconv also.

Currently all the buildtools/wafsamba/samba_conftests.py charset tests fail but the HAVE_CHARSET_ defines are never used, so the tests are quite useless at the moment. Errors will pop up at runtime, when unsupported charsets are being used only.

(In reply to Jeremy Allison from comment #17)
ping acknowledged.

I am hoping to get onto it in the enxt few days.

Created attachment 16655 [details]
WIP patch for master

I'll put this here because it is dinner time, and as I was going to make the knownfail to bridge the tests and the fix, I found one test crashed instead of failed. Which I know shouldn't be a surprise, given what it's testing.

So I am putting this here lest I forget about it again.

Created attachment 16656 [details]
patch for master

dinner is late. Here is the working patch.

This bug was referenced in samba master:

50047588c0c8da2e1ffa0b08a8dc5d31e49f6a3b
1ea1816629104e16d9b180bee8f4ccc6292869ed

This will backport easily if we want it.

looks like we can close this now.