SiteHPKPState::SiteHPKPState (38.0.1\security\manager\boot\src\nsSiteSecurityService.cpp line 126) calls PR_sscanf using an uncounted %s specifier on lines 142-44: 142: int32_t matches = PR_sscanf(aStateString.get(), "%lld,%lu,%lu,%s", 143: &mExpireTime, &hpkpState, 144: &hpkpIncludeSubdomains, mergedHPKPins); The call is currently safe because the code checks aStateString's length on line 137: 134: char mergedHPKPins[MaxMergedHPKPPinSize]; 135: memset(mergedHPKPins, 0, MaxMergedHPKPPinSize); 136: 137: if (aStateString.Length() >= MaxMergedHPKPPinSize) { 138: SSSLOG(("SSS: Cannot parse PKPState string, too large\n")); 139: return; 140: } but using the uncounted %s specifier is very bad practice because of the possibility of buffer overruns. Marked as a security bug to avoid tipping off attackers by posting more than one sscanf bug in a short period of time.

Looks like this PR_sscanf usage was added in mozilla-central changeset 605c11a57482. Tagging that bug's reviewer, keeler, for needinfo to determine next-action here.

[marking as blocking bug 787133, which added this code; normally I'd be a bit hesitant to do this for a sec bug, to avoid leaking information that there's-a-sec-vulnerability-in-this-patch-somewhere on the old bug. But in this case, comment 0 indicates that this is "currently safe" so I think we're OK.]

We should probably re-work that code to avoid PR_sscanf or at least make it more obviously safe. This isn't a security-sensitive bug, though.

Maybe use the parser from bug 1024056?

Comment on attachment 8867490 [details] Bug 1174555 - Stop using PR_sscanf() in nsSiteSecurityService.cpp. https://reviewboard.mozilla.org/r/139026/#review142606 LGTM - just a couple of comments. ::: security/manager/ssl/nsSiteSecurityService.cpp:79 (Diff revision 1) > +{ > +public: > + // "=+/" are the extra characters required to make Base64 strings be > + // considered "words". > + SSSTokenizer(const nsACString& source) > + : Tokenizer(source, nullptr, "=+/") Does this include digits in words? It's not super clear to me, but looking at the tests I suppose it does: https://dxr.mozilla.org/mozilla-central/source/xpcom/tests/gtest/TestTokenizer.cpp#107 In any case, it seems that test_sss_savestate.js may be incomplete - the only key hash it tests is `AAAA....=` - it would be nice to expand that to include multiple key hashes that have digits and the extra characters. ::: security/manager/ssl/nsSiteSecurityService.cpp:137 (Diff revision 1) > + return false; > + } > + > + for (uint32_t i = 0; i < mergedKeys.Length(); i += SHA256Base64Len) { > + nsAutoCString key(Substring(mergedKeys, i, SHA256Base64Len)); > + if (stringIsBase64EncodingOf256bitValue(key)) { This is a preexisting issue, but it's a little odd to me that we silently discard bad key hashes here, since this data is only supposed to come from Firefox itself. If someone does modify an HPKP entry manually and gets it wrong, they could unintentionally brick a site (then again, they could brick a site with a correctly formatted hash that was just wrong). I think we should be consistent, though - my understanding is that we accept or reject whole entries in terms of every other field, so that's probably what we should do here. ::: security/manager/ssl/nsSiteSecurityService.cpp:299 (Diff revision 1) > + switch (state) { > + case SecurityPropertyKnockout: > + case SecurityPropertySet: > + case SecurityPropertyUnset: > + break; > + case SecurityPropertyNegative: Seems like we don't have to have a full switch statement just to check that `state != SecurityPropertyNegative`, right? (Instead we should just include a comment saying that isn't a valid state for HPKP.) ::: security/manager/ssl/nsSiteSecurityService.cpp:343 (Diff revision 1) > , mOriginAttributes(aOriginAttributes) > , mExpireTime(0) > , mState(SecurityPropertyUnset) > , mIncludeSubdomains(false) > { > - uint32_t hpkpState = 0; > + // Just as a precaution, don't parse state strings that are too large. I think this was an optimization based on that the backing storage limits these strings to 1024 bytes. Since we're not using a fixed-size stack buffer any longer, we don't really need to do this check.

Comment on attachment 8867491 [details] Bug 1174555 - Clean up some SiteSecurityService state file related tests. https://reviewboard.mozilla.org/r/139028/#review142722 ::: security/manager/ssl/tests/unit/head_psm.js:872 (Diff revision 1) > // prepending a "0" solves the problem. > return Array.from(data, (c, i) => ("0" + data.charCodeAt(i).toString(16)).slice(-2)).join(""); > } > + > +/** > + * @param {String[]} lines Maybe document that this function adds no line delimiter? (as in, line breaks are the responsibility of the caller)

Comment on attachment 8867492 [details] Bug 1174555 - Improve state string parsing test coverage. https://reviewboard.mozilla.org/r/139030/#review142746 Cool - this might be a good place to add a testcase for a key hash that has "+/" characters in it (looks like we now have one with numbers).

(In reply to David Keeler [:keeler] (use needinfo?) from comment #8) > > + : Tokenizer(source, nullptr, "=+/") > > Does this include digits in words? It's not super clear to me, but looking If a token starts with digits, like "01ab23+=" then with "=+/" as additional chars it will be parsed as INTEGER:1 and WORD:"ab23+=". We could add an API to Tokenizer that changes the list of additional chars at runtime, so you can add "012...9" to take digits as word token starters.

(In reply to Honza Bambas (:mayhemer) from comment #11) > If a token starts with digits, like "01ab23+=" then with "=+/" as additional > chars it will be parsed as INTEGER:1 and WORD:"ab23+=". Thanks, you've probably saved us some confused debugging in the future. > We could add an API > to Tokenizer that changes the list of additional chars at runtime, so you > can add "012...9" to take digits as word token starters. I've found a workaround for now, but that would be nice.

Comment on attachment 8867490 [details] Bug 1174555 - Stop using PR_sscanf() in nsSiteSecurityService.cpp. https://reviewboard.mozilla.org/r/139026/#review142606 > Does this include digits in words? It's not super clear to me, but looking at the tests I suppose it does: https://dxr.mozilla.org/mozilla-central/source/xpcom/tests/gtest/TestTokenizer.cpp#107 > > In any case, it seems that test_sss_savestate.js may be incomplete - the only key hash it tests is `AAAA....=` - it would be nice to expand that to include multiple key hashes that have digits and the extra characters. Partially, as :mayhemer answered in comment 11. Unfortunately this behaviour means I had to find a less-nice workaround solution instead. > I think this was an optimization based on that the backing storage limits these strings to 1024 bytes. Since we're not using a fixed-size stack buffer any longer, we don't really need to do this check. Yeah, I was being overly paranoid.

Comment on attachment 8867491 [details] Bug 1174555 - Clean up some SiteSecurityService state file related tests. https://reviewboard.mozilla.org/r/139028/#review142722 > Maybe document that this function adds no line delimiter? (as in, line breaks are the responsibility of the caller) I actually decided that it would make more sense for the function to add line breaks, so I did that and added documentation.

Comment on attachment 8867492 [details] Bug 1174555 - Improve state string parsing test coverage. https://reviewboard.mozilla.org/r/139030/#review142746 I added a few test cases including these characters, and a test case that would've caught the undesired "string starting with digits doesn't count as a word" behaviour.

keeler: I would appreciate it if you could take a quick look to confirm the workaround I went with looks sane.

Comment on attachment 8867490 [details] Bug 1174555 - Stop using PR_sscanf() in nsSiteSecurityService.cpp. https://reviewboard.mozilla.org/r/139026/#review144688 Looks good. I actually think I was wrong about a previous review comment, though - see below. ::: security/manager/ssl/nsSiteSecurityService.cpp:129 (Diff revisions 1 - 2) > + // run time, meaning parsing digits as a number will fail. > MOZ_MUST_USE bool > - ReadSHA256Keys(/*out*/ nsTArray<nsCString>& keys) > + ReadUntilEOFAsSHA256Keys(/*out*/ nsTArray<nsCString>& keys) > { > nsAutoCString mergedKeys; > - if (!ReadWord(mergedKeys)) { > + if (!ReadUntil(Token::EndOfFile(), mergedKeys, EXCLUDE_LAST)) { I think this is reasonable. ::: security/manager/ssl/nsSiteSecurityService.cpp:300 (Diff revisions 1 - 2) > > if (!tokenizer.ReadState(state)) { > return false; > } > - switch (state) { > - case SecurityPropertyKnockout: > + > + // SecurityPropertyNegative isn't a valid state for HPKP. Ah, shoot - I actually think you did this correctly the first time. It's probably safest to implement this as a whitelist rather than a blacklist, so that if someone adds a new value, they'll have to make the choice of whether or not it's a valid HPKP state. Sorry about that. ::: security/manager/ssl/nsSiteSecurityService.cpp:318 (Diff revisions 1 - 2) > if (!tokenizer.CheckChar(',')) { > return false; > } > > if (state == SecurityPropertySet) { > - if (!tokenizer.ReadSHA256Keys(sha256keys)) { > + return tokenizer.ReadUntilEOFAsSHA256Keys(sha256keys); Maybe comment that this reads to the end of the input?

Thanks for the review! https://treeherder.mozilla.org/#/jobs?repo=try&revision=68e2de7f55ff000062ae7108144a1a0834d62952&selectedJob=100715910 (Some Linux64 xpcshell jobs still aren't done yet, but it doesn't matter because due to a quirk with the chunking logic, each of the xpcshell chunks runs all of the PSM tests.)

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/4eb036bf55f0 Stop using PR_sscanf() in nsSiteSecurityService.cpp. r=keeler https://hg.mozilla.org/integration/autoland/rev/ac1fab84c97a Clean up some SiteSecurityService state file related tests. r=keeler https://hg.mozilla.org/integration/autoland/rev/d63eff7188da Improve state string parsing test coverage. r=keeler

https://hg.mozilla.org/mozilla-central/rev/4eb036bf55f0 https://hg.mozilla.org/mozilla-central/rev/ac1fab84c97a https://hg.mozilla.org/mozilla-central/rev/d63eff7188da