Closed Bug 1344380 (CVE-2017-5461) Opened 3 years ago Closed 3 years ago

Write beyond bounds caused by bugs in Base64 de/encoding in nssb64d.c and nssb64e.c

Categories

(NSS :: Libraries, defect)

defect
Not set
normal

Tracking

(firefox-esr4553+ fixed, firefox52 wontfix, firefox-esr5253+ fixed, firefox53+ fixed, firefox54+ fixed, firefox55+ fixed)

RESOLVED FIXED
Tracking Status
firefox-esr45 53+ fixed
firefox52 --- wontfix
firefox-esr52 53+ fixed
firefox53 + fixed
firefox54 + fixed
firefox55 + fixed

People

(Reporter: q1, Assigned: franziskus)

References

Details

(Keywords: sec-critical, Whiteboard: [post-critsmash-triage][adv-main53+][adv-esr45.9+][adv-esr52.1+])

PL_Base64MaxDecodedLength() (security\nss\lib\util\nssb64d.c) can cause an integer overflow. If it does, its caller(s) will allocate a buffer too small for the result of a Base64 decoding operation, then possibly overrun it.

For example, PL_Base64DecodeBuffer() (security\nss\lib\util\nssb64d.c) uses the result of PL_Base64MaxDecodedLength() to check whether a caller-provided buffer is long enough, or to allocate a buffer if the caller does not provide one. In either case, the calculated length is inadequate if the length of the encoded string is >= 0x55555556. Once it has calculated the (potentially incorrect) length, it passes it and the corresponding buffer to pl_base64_decode_buffer() (same module) to do the decoding. That function incrementally checks against the provided length (data->output_buflen) but (another bug!) *only in debug builds (i.e. via a PR_ASSERT())*.

The upshot is that PL_Base64DecodeBuffer() might be vulnerable to the same kind of buffer overrun bug as in https://bugzilla.mozilla.org/show_bug.cgi?id=1344081 : an attack that overruns a heap buffer, by an attacker-determined amount, with 3-byte granularity.

There is a similar issue in PL_Base64MaxEncodedLength() and its associated functions for Base64 encoding.

I will try to manifest these bugs and post a POC if and when I do.

The bugs are in lines 373 and 254 (nssb64d.c):

370: static PRUint32
371: PL_Base64MaxDecodedLength (PRUint32 size)
372: {
373:     return ((size * 3) / 4);
374: }

219: static PRStatus
220: pl_base64_decode_buffer (PLBase64Decoder *data, const unsigned char *in,
221: 			 PRUint32 length)
222: {
223:     unsigned char *out = data->output_buffer;
224:     unsigned char *token = data->token;
225:     int i, n = 0;
226: 
227:     i = data->token_size;
228:     data->token_size = 0;
229: 
230:     while (length > 0) {
231: 	while (i < 4 && length > 0) {
...
254: 	   PR_ASSERT((out - data->output_buffer + 3) <= data->output_buflen);

and 288 and potentially 300 (nssb64e.c):

280: static PRUint32
281: PL_Base64MaxEncodedLength (PRUint32 size, PRUint32 line_length)
282: {
283:     PRUint32 tokens, tokens_per_line, full_lines, line_break_chars, remainder;
284: 
285:     tokens = (size + 2) / 3;
286: 
287:     if (line_length == 0)
288: 	      return tokens * 4;
289: 
290:     if (line_length < 4)	/* too small! */
291: 	      line_length = 4;
292: 
293:     tokens_per_line = line_length / 4;
294:     full_lines = tokens / tokens_per_line;
295:     remainder = (tokens - (full_lines * tokens_per_line)) * 4;
296:     line_break_chars = full_lines * 2;
297:     if (remainder == 0)
298: 	   line_break_chars -= 2;
299: 
300:     return (full_lines * tokens_per_line * 4) + line_break_chars + remainder;
301: }
Summary: Write beyond bounds caused by bugs in Base64 decoding in nssb64d.c → Write beyond bounds caused by bugs in Base64 decoding in nssb64d.c and nssb64e.c
"There is a similar issue in PL_Base64MaxEncodedLength() and its associated functions for Base64 encoding" should be followed by " (nssb64e.c)".
Summary: Write beyond bounds caused by bugs in Base64 decoding in nssb64d.c and nssb64e.c → Write beyond bounds caused by bugs in Base64 de/encoding in nssb64d.c and nssb64e.c
Assignee: nobody → nobody
Component: Security → Libraries
Product: Core → NSS
Version: 51 Branch → trunk
Flags: needinfo?(dveditz)
Group: core-security → crypto-core-security
Flags: needinfo?(dveditz)
Flags: needinfo?(dveditz)
Flags: sec-bounty?
Adding Fransziskus, an NSS peer, per his request.
Assignee: nobody → franziskuskiefer
This seems to be used a bunch in our codebase. Bug 1344081 and bug 1344305 are due to this, but it's also used as the basis for Base64Decode() in xpcom/io which seems to be used all over the place.
Flags: sec-bounty?
Flags: sec-bounty+
Flags: needinfo?(dveditz)
Keywords: sec-critical
(In reply to Daniel Veditz [:dveditz] from comment #4)
> This seems to be used a bunch in our codebase. Bug 1344081 and bug 1344305
> are due to this, but it's also used as the basis for Base64Decode() in
> xpcom/io which seems to be used all over the place.

Base64Decode() in xpcom/io uses NSPR's base64 functions, not NSS's.
https://hg.mozilla.org/projects/nss/rev/77a5bb81dbaac5b03266a64ff981c156b61c8931
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.31
I assume we'll want to backport this to 3.28/3.29/3.30?
Flags: needinfo?(franziskuskiefer)
Marking this as affected for current versions and esr. Should this be included in whatever NSS update we may do across all branches?
Blocks: 1344368
Group: crypto-core-security → core-security-release
(In reply to Ryan VanderMeulen [:RyanVM] from comment #8)
> I assume we'll want to backport this to 3.28/3.29/3.30?

If there's another 3.28 point release, we'll add this bug (see bug 1344368). As for 3.29 and 3.30, we'll wait another two weeks to see if there's more we have to get in. (Leaving the ni? here as a reminder.)
(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #10)
> If there's another 3.28 point release, we'll add this bug (see bug 1344368).

This bug would be enough to be a driver for another 3.28 point release, wouldn't it? Need to figure out what to do about ESR45 as well, which is on 3.21.3 at the moment.
> passes it and the corresponding buffer to pl_base64_decode_buffer() (same module) to do the decoding. That function
> incrementally checks against the provided length (data->output_buflen) but (another bug!) *only in debug builds (i.e. via a 
> PR_ASSERT())*.

The patch appears not to fix this portion of the original bug.
> This bug would be enough to be a driver for another 3.28 point release, wouldn't it?

Yeah, we might make a point release based on this one. But that wouldn't get picked up by Firefox as long as there's no point release for 52.

> >  pl_base64_decode_buffer(). That function incrementally checks against the provided length (data->output_buflen) only in debug builds
> The patch appears not to fix this portion of the original bug.

It only asserts, that's correct. But with PL_Base64MaxDecodedLength computing the correct length this really is a sanity check. Do you have inputs that trigger this assert?
Flags: needinfo?(franziskuskiefer)
(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #13)
> > >  pl_base64_decode_buffer(). That function incrementally checks against the provided length (data->output_buflen) only in debug builds
> > The patch appears not to fix this portion of the original bug.
> 
> It only asserts, that's correct. But with PL_Base64MaxDecodedLength
> computing the correct length this really is a sanity check....

OK, I see: pl_base64_decode_buffer() is |static|, so it's only ever called by other functions in nssb64d.c, and both current callers call PL_Base64MaxDecodedLength() first. So it's OK.
(In reply to Tim Taubert [:ttaubert] from comment #15)
> Landed on the NSS 3.28.4 branch:
> 
> https://hg.mozilla.org/projects/nss/rev/6eb39ead39e0

Are we taking this in Firefox 53?
Flags: needinfo?(ttaubert)
(In reply to Al Billings [:abillings] from comment #16)
> (In reply to Tim Taubert [:ttaubert] from comment #15)
> > Landed on the NSS 3.28.4 branch:
> > 
> > https://hg.mozilla.org/projects/nss/rev/6eb39ead39e0
> 
> Are we taking this in Firefox 53?

Yeah. Franziskus is currently preparing releases and uplift requests.
Flags: needinfo?(ttaubert)
Blocks: 1353740
Blocks: 1353742
Blocks: 1353748
This should be fixed in beta by the NSS upgrade in bug 1353740 (for the beta 10 build tomorrow)
Will you assign a CVE?
What is the trigger here? Base64 user input, like certificates, private key material or something else?
The trigger can be anything that's handed to the base64 decoder/encoder. So yes, also certs etc.
Hi, can you tell us if this patch addresses the issue sufficiently?
Flags: needinfo?(q1)
Flags: qe-verify?
Whiteboard: [post-critsmash-triage]
(In reply to Matt Wobensmith [:mwobensmith][:matt:] from comment #22)
> Hi, can you tell us if this patch addresses the issue sufficiently?

Yes, the patch appears correct.
Flags: needinfo?(q1)
Thank you very much.
Flags: qe-verify? → qe-verify-
Alias: CVE-2017-5461
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main53+][adv-esr45.9+][adv-esr52.1+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.