Bug 1344380 (CVE-2017-5461)

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

RESOLVED FIXED in Firefox -esr45

Status

RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: q1, Assigned: franziskus)

Tracking

({sec-critical})

trunk
3.31
sec-critical
Dependency tree / graph
Bug Flags:
sec-bounty +
qe-verify -

Firefox Tracking Flags

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

Details

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

(Reporter)

Description

2 years ago
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: }
(Reporter)

Updated

2 years ago
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
(Reporter)

Comment 1

2 years ago
"There is a similar issue in PL_Base64MaxEncodedLength() and its associated functions for Base64 encoding" should be followed by " (nssb64e.c)".
(Reporter)

Updated

2 years ago
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

Updated

2 years ago
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)

Updated

2 years ago
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

Comment 6

2 years ago
(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.
(Assignee)

Comment 7

2 years ago
https://hg.mozilla.org/projects/nss/rev/77a5bb81dbaac5b03266a64ff981c156b61c8931
Status: NEW → RESOLVED
Last Resolved: 2 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?
status-firefox52: --- → affected
status-firefox53: --- → affected
status-firefox54: --- → affected
status-firefox55: --- → fixed
status-firefox-esr45: --- → affected
status-firefox-esr52: --- → affected
tracking-firefox-esr45: --- → ?
tracking-firefox-esr52: --- → ?
(Assignee)

Updated

2 years ago
Blocks: 1344368
Group: crypto-core-security → core-security-release
(Assignee)

Comment 10

2 years ago
(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.
(Reporter)

Comment 12

2 years ago
> 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.
(Assignee)

Comment 13

2 years ago
> 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)
(Reporter)

Comment 14

2 years ago
(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)
(Assignee)

Updated

2 years ago
Blocks: 1353740
(Assignee)

Updated

2 years ago
Blocks: 1353742
(Assignee)

Updated

2 years ago
Blocks: 1353748
This should be fixed in beta by the NSS upgrade in bug 1353740 (for the beta 10 build tomorrow)
tracking-firefox53: --- → +
tracking-firefox54: --- → +
tracking-firefox55: --- → +

Comment 19

2 years ago
Will you assign a CVE?
status-firefox52: affected → wontfix
status-firefox53: affected → fixed
status-firefox54: affected → fixed
status-firefox-esr45: affected → fixed
status-firefox-esr52: affected → fixed

Comment 20

2 years ago
What is the trigger here? Base64 user input, like certificates, private key material or something else?
(Assignee)

Comment 21

2 years ago
The trigger can be anything that's handed to the base64 decoder/encoder. So yes, also certs etc.
tracking-firefox-esr45: ? → 53+
tracking-firefox-esr52: ? → 53+
Hi, can you tell us if this patch addresses the issue sufficiently?
Flags: needinfo?(q1)
Flags: qe-verify?
Whiteboard: [post-critsmash-triage]
(Reporter)

Comment 23

2 years ago
(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.