1311319 - Do not call PR_GetErrorText() when PR_GetErrorTextLength() is 0

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Tooru Fujisawa [:arai]]

derived from bug 1283710.

PR_GetErrorText() does nothing if PR_GetErrorTextLength() is 0.
https://dxr.mozilla.org/mozilla-central/rev/01ab78dd98805e150b0311cce2351d5b408f3001/nsprpub/pr/src/misc/prerror.c#67
> PR_IMPLEMENT(PRInt32) PR_GetErrorText(char *text)
> {
>     PRThread *thread = PR_GetCurrentThread();
>     if (0 != thread->errorStringLength)
>         memcpy(text, thread->errorString, thread->errorStringLength+1);
>     return thread->errorStringLength;
> }  /* PR_GetErrorText */


If the caller doesn't zero the buffer, it results in uninitialized unterminated string.

https://dxr.mozilla.org/mozilla-central/rev/01ab78dd98805e150b0311cce2351d5b408f3001/js/src/ctypes/Library.cpp#158
>     char* error = (char*) JS_malloc(cx, PR_GetErrorTextLength() + 1);
>     if (error)
>       PR_GetErrorText(error);


so far I don't see any other critical case tho, marking as s-s just in case.

Comment 1

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Do not call PR_GetErrorText() when PR_GetErrorTextLength() is 0.

Changed to use "Cannot get error from NSPR." as default message, that I saw here
http://searchfox.org/mozilla-central/rev/d96317a351af8aa78ab9847e7feed964bbaac7d7/dom/plugins/base/nsPluginsDirUnix.cpp#49

Comment 2

[Tooru Fujisawa [:arai]]

Attachment: Part 2: Use Latin1 variant of error reporting if OS error is not ASCII.

maybe I should file another bug for this tho, posting here as it's adjacent code to Part 1, and somewhat related.

`error` there is OS error, that's not guaranteed to be ASCII or UTF-8.
Now we can check the encoding by StringIsASCII, added Latin1 branch for non-ascii case.

Comment 3

[Tooru Fujisawa [:arai]]

Comment on attachment 8802471 [details] [diff] [review]
Part 1: Do not call PR_GetErrorText() when PR_GetErrorTextLength() is 0.

sorry, this was wrong one.
will post correct one shortly.

Comment 4

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Do not call PR_GetErrorText() when PR_GetErrorTextLength() is 0.

Changed to use the same way as other case.
now it allocates string on stack with default message, and overwrites only if there's OS error and it fits in the string.

Comment 5

[Tooru Fujisawa [:arai]]

Attachment: Part 2: Use Latin1 variant of error reporting if OS error is not ASCII.

rebased, and fixed JS:: namespace

Comment 6

[Andrew McCreight [:mccr8]]

(In reply to Tooru Fujisawa [:arai] from comment #0)
> so far I don't see any other critical case tho, marking as s-s just in case.

I'm going to mark this bug sec-audit because of this. If you did find some bad cases, please adjust the rating or clear it and we'll look at it in triage again.

Comment 7

[Tooru Fujisawa [:arai]]

found 2 other cases that may hit this issue.

https://dxr.mozilla.org/mozilla-central/rev/99a239e1866a57f987b08dad796528e4ea30e622/testing/mochitest/ssltunnel/ssltunnel.cpp#1560-1564
>     int32_t errorlen = PR_GetErrorTextLength();
>     char* err = new char[errorlen+1];
>     PR_GetErrorText(err);
>     LOG_ERROR(("Failed to init NSS: %s", err));
>     delete[] err;

https://dxr.mozilla.org/mozilla-central/rev/99a239e1866a57f987b08dad796528e4ea30e622/nsprpub/pr/tests/errset.c#138-149
>         char    msg[256];
> 
>         PR_SetError( errnum, -5 );
>         err = PR_GetError();
>         PR_ASSERT( err == errnum );
>         err = PR_GetOSError();
>         PR_ASSERT( err == -5 );
>         PR_SetErrorText( strlen(errcodes[errnum].errname), errcodes[errnum].errname );
>         len1 = PR_GetErrorTextLength();
>         len2 = PR_GetErrorText( msg );
>         PR_ASSERT( len1 == len2 );
>         printf("%5.5d -- %s\n", errnum, msg );

according to the path, both are testing code.

Comment 8

[Tooru Fujisawa [:arai]]

js/src/ctypes/Library.cpp's case will be sec-low:
  * chrome-priv is necessary to call this function
  * this is in error handling, and web content cannot cause the error
  * this can expose uninitialized memory content as error message string,
    but the error message isn't available to web content

nsprpub/pr/tests/errset.c wasn't a bug.  the text is set by the code and the length is always non-zero

I'm not sure about testing/mochitest/ssltunnel/ssltunnel.cpp

Comment 9

[Tooru Fujisawa [:arai]]

oops, changed by mistake.
still need audit for ssltunnel.cpp

Comment 10

[Jeff Walden [:Waldo]]

(In reply to Tooru Fujisawa [:arai] from comment #9)
> still need audit for ssltunnel.cpp

Note the comment atop ssltunnel.cpp:

/*
 * WARNING: DO NOT USE THIS CODE IN PRODUCTION SYSTEMS.  It is highly likely to
 *          be plagued with the usual problems endemic to C (buffer overflows
 *          and the like).  We don't especially care here (but would accept
 *          patches!) because this is only intended for use in our test
 *          harnesses in controlled situations where input is guaranteed not to
 *          be malicious.
 */

Whatever problems ssltunnel.cpp might or might not have, effectively they're in test-only code and should be deprioritized accordingly.

Comment 11

[Tooru Fujisawa [:arai]]

Thanks!
will prepare a patch for ssltunnel.cpp.

Comment 12

[Tooru Fujisawa [:arai]]

Attachment: Part 3: Do not call PR_GetErrorText() when PR_GetErrorTextLength() is 0 in ssltunnel.cpp.

Comment 13

[Jeff Walden [:Waldo]]

Comment on attachment 8802492 [details] [diff] [review]
Part 2: Use Latin1 variant of error reporting if OS error is not ASCII.

Review of attachment 8802492 [details] [diff] [review]:
-----------------------------------------------------------------

Nice approach!

Comment 14

[Jeff Walden [:Waldo]]

Comment on attachment 8803223 [details] [diff] [review]
Part 3: Do not call PR_GetErrorText() when PR_GetErrorTextLength() is 0 in ssltunnel.cpp.

Review of attachment 8803223 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/mochitest/ssltunnel/ssltunnel.cpp
@@ +1560,5 @@
>      int32_t errorlen = PR_GetErrorTextLength();
> +    if (errorlen) {
> +      char* err = new char[errorlen+1];
> +      PR_GetErrorText(err);
> +      LOG_ERROR(("Failed to init NSS: %s", err));

Do

  auto err = mozilla::MakeUnique<char[]>(errorlen + 1);
  PR_GetErrorText(err.get());
  LOG_ERROR(("Failed to init NSS: %s", err.get()));

so you can remove the delete[].  (Technically this is less efficient, because MakeUnique value-initializes the array, which for scalar elements zero-initializes them.  But it's test code, who cares.  :-) )

Comment 15

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1be47494e723521e96f5541d985c63a77dd225ef
Bug 1311319 - Part 1: Do not call PR_GetErrorText() when PR_GetErrorTextLength() is 0. r=sfink

https://hg.mozilla.org/integration/mozilla-inbound/rev/5cd94a51349c104067c7dbd7b7f76d90c95fda0b
Bug 1311319 - Part 2: Use Latin1 variant of error reporting if OS error is not ASCII. r=jwalden

https://hg.mozilla.org/integration/mozilla-inbound/rev/4c77ff67311dd0cf52bc211ce0cb1d118000f3d4
Bug 1311319 - Part 3: Do not call PR_GetErrorText() when PR_GetErrorTextLength() is 0 in ssltunnel.cpp. r=jwalden

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1be47494e723
https://hg.mozilla.org/mozilla-central/rev/5cd94a51349c
https://hg.mozilla.org/mozilla-central/rev/4c77ff67311d

Comment 17

[Tooru Fujisawa [:arai]]

Part 2 don't need backport.
Part 1 and 3 might be nice to backport to beta and aurora.
will check and prepare.

Comment 18

[Tooru Fujisawa [:arai]]

Attachment: (mozilla-aurora/beta) Part 1: Do not call PR_GetErrorText() when PR_GetErrorTextLength() is 0. r=sfink

Approval Request Comment
> [Feature/regressing bug #]
Bug 1131424 (Part 1) and Bug 426867 (Part 3).

> [User impact if declined]
Possible crash.

> [Describe test coverage new/current, TreeHerder]
tested in automated test.

> [Risks and why]
Low.  Both add fallback message for erroneous case.

> [String/UUID change made/needed]
None

Comment 19

[Tooru Fujisawa [:arai]]

Attachment: (mozilla-aurora/beta) Part 3: Do not call PR_GetErrorText() when PR_GetErrorTextLength() is 0 in ssltunnel.cpp. r=jwalden

Comment 20

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8803717 [details] [diff] [review]
(mozilla-aurora/beta) Part 1: Do not call PR_GetErrorText() when PR_GetErrorTextLength() is 0. r=sfink

This is a sec-low, ok for Aurora51, wontfix for beta50.

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1e0748e1dc35
https://hg.mozilla.org/releases/mozilla-aurora/rev/b82a286b69e0