User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/66.0.3359.139 Safari/537.36 Steps to reproduce: The code is primarily taken from TestCSPParser.cpp. Note that the dictionary is essential, otherwise coverage barely moves. I've run it for a few CPU hours, and coverage increases nicely and steadily. There is only a minor problem. After about 100K runs, libFuzzer reports this. Any idea what could cause it here? INFO: libFuzzer disabled leak detection after every mutation. Most likely the target function accumulates allocated memory in a global state w/o actually leaking it. You may try running this binary with -trace_malloc=[12] to get a trace of mallocs and frees. If LeakSanitizer is enabled in this process it will still run on the process shutdown. The suggested flags report malloc and free, and many mallocs have no free. About 100 per single run. Like this one. #6 0x35e97d in moz_xmalloc mozilla-central/memory/mozalloc/mozalloc.cpp:70:17 #7 0x7f59c379b3e0 in operator new mozilla-central/obj-fuzz/dist/include/mozilla/mozalloc.h:156:12 #8 0x7f59c379b3e0 in mozilla::Preferences::AddBoolVarCache(bool*, nsTSubstring<char> const&, bool, bool) mozilla-central/modules/libpref/Preferences.cpp:4789 #9 0x7f59c39ee344 in nsresult mozilla::Preferences::AddBoolVarCache<33>(bool*, char const (&) [33], bool, bool) mozilla-central/obj-fuzz/dist/include/mozilla/Preferences.h:421:12 #10 0x7f59c9535d67 in nsCSPParser::nsCSPParser(nsTArray<nsTArray<nsTString<char16_t> > >&, nsIURI*, nsCSPContext*, bool) mozilla-central/dom/security/nsCSPParser.cpp:90:5 #11 0x7f59c952a53d in nsCSPParser::parseContentSecurityPolicy(nsTSubstring<char16_t> const&, nsIURI*, bool, nsCSPContext*, bool) mozilla-central/dom/security/nsCSPParser.cpp:1278:15 #12 0x7f59c9529eb7 in nsCSPContext::AppendPolicy(nsTSubstring<char16_t> const&, bool, bool) mozilla-central/dom/security/nsCSPContext.cpp:414:25 #13 0x7f59ce40ada5 in LLVMFuzzerTestOneInput(unsigned char const*, unsigned long) mozilla-central/dom/security/fuzztest/csp_fuzzer.cpp:44:8

I'm not certain if this is a Core: Memory Allocator issue or a Core: DOM Security issue. Please correct the component as necessary. Thanks.

It's unclear exactly what's going on, but I've filed 1476820 to make these prefs static, which will eliminate the need for the AddBoolVarCache() calls, and hopefully will eliminate the reports.

Sorry for the delay here, will take a look shortly. As the leak will be handled in bug 1476820 (thanks :njn!), moving this to the DOM component.

Comment on attachment 8991791 [details] [diff] [review] ContentSecurityPolicyParser Review of attachment 8991791 [details] [diff] [review]: ----------------------------------------------------------------- Thanks again for investing time to help us! This looks really good from the fuzzing side, minor comments inline. Forwarding the actual code review and approval request to Christoph as the CSP module owner. ::: dom/security/fuzztest/csp_fuzzer.cpp @@ +33,5 @@ > + > + if (csp->SetRequestContext(nullptr, selfURIPrincipal) != NS_OK) > + return 0; > + > + // Input must be null-terminated. Not sure if there is a more elegant way to do this using one of our string classes/helpers. There is > NS_ConvertASCIItoUTF16(const char* aCString, uint32_t aLength) which might already do what you want. @@ +34,5 @@ > + if (csp->SetRequestContext(nullptr, selfURIPrincipal) != NS_OK) > + return 0; > + > + // Input must be null-terminated. > + char* text = (char*)malloc(size + 1); If this code remains, it probably needs a NULL check after the malloc. ::: dom/security/fuzztest/csp_fuzzer.dict @@ +1,1 @@ > +# taken from nsCSPParser.cpp and nsCSPUtils.{cpp,h} Does it make sense to take some of the values from the CSPParser gtest as well? It seems to have more interesting values. Just a suggestion :)

Marking s-s until this has received more fuzzing, so we don't easily expose potential security problems.

Bug 1476820 has landed on mozilla-inbound.

Comment on attachment 8991791 [details] [diff] [review] ContentSecurityPolicyParser Review of attachment 8991791 [details] [diff] [review]: ----------------------------------------------------------------- Yeah, I am fine with adding those fuzzing bits to the tree. Please address decoder's comments and you are good to go. r=me As decoder pointed out, probably worth taking a look at TestCSPParser.cpp and grep a few values from there! Thanks for doing this!

I'm testing this locally right now at about 9k iterations/s and will leave it running till Monday. If nothing drops out of that, then I think it'll be safe to land on m-c.

> Not sure if there is a more elegant way to do this using one of our string > classes/helpers. There is > > > NS_ConvertASCIItoUTF16(const char* aCString, uint32_t aLength) > > which might already do what you want. Done. I was actually looking for sth. like that. Thanks! > Does it make sense to take some of the values from the CSPParser gtest as > well? It seems to have more interesting values. Just a suggestion :) There aren't many additional values, except for a few I had missed originally in the other files. I went ahead and pulled the values from the spec. (Some seem not yet supported, but that's potential future coverage then.) PS. The leaks were fixed by bug 1476820. Thanks.

> PS. The leaks were fixed by bug 1476820. Thanks. Good to hear! Thank you for confirming.

Now that oss-fuzz is working correctly, and the target can start fuzzing right away, I think it should be good to go. Please confirm. (I'll make some additional minor changes to the dictionary, but otherwise that's the patch.)

(In reply to pdknsk from comment #12) > Now that oss-fuzz is working correctly, and the target can start fuzzing > right away, I think it should be good to go. Please confirm. (I'll make some > additional minor changes to the dictionary, but otherwise that's the patch.) Sure, go ahead. Please mark the first patch here as obsoleted by the second and carry over the r+/sr+ on the new patch, then land it by requesting checkin. Local fuzzing didn't reveal anything here, so I think we should be good to go.

(I can't add the review flags or I don't know how.)