[Static Analysis][Result of operation is garbage or undefined] Function PR_LocalTimeParameters from nsprpub/pr/src/misc/prtime.c can create variable localTime with garbage content

RESOLVED FIXED in 4.15

Status

defect
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: andi, Assigned: kaie)

Tracking

(Blocks 1 bug, {clang-analyzer})

Dependency tree / graph

Firefox Tracking Flags

(firefox45 affected)

Details

Attachments

(2 attachments, 4 obsolete attachments)

The Static Analysis tool Scan-Build added that variable localTime can have garbage content if localtime() function called from MT_safe_localtime fails.
Posted patch Bug 1227497.diff (obsolete) — Splinter Review
Component: Security → NSPR
Product: Core → NSPR
Version: Trunk → 4.11
Andi, what should we do with this one ? :)
Flags: needinfo?(bpostelnicu)
Attachment #8789706 - Flags: review?(kaie)
Attachment #8691353 - Attachment is obsolete: true
Flags: needinfo?(bpostelnicu)
I think a clarification is needed for this bug. MT_safe_localtime fails only if :

>>    tmPtr = localtime(clock);
>>
>>#if defined(WIN16) || defined(XP_OS2)
>>    if ( (PRInt32) *clock < 0 ||
>>         ( (PRInt32) *clock == 0 && tmPtr->tm_year != 70))
>>        result = NULL;
>>    else
>>        *result = *tmPtr;
>>#else
>>    if (tmPtr) {
>>        *result = *tmPtr;
>>    } else {
>>        result = NULL;
>>    }
>>#endif /* WIN16 */

localtime returns a null ptr
Because you attached a diff against the firefox source tree, a general reminder:
NSPR changes must be commited to the separate NSPR repository, and firefox consumes updates to NSPR as new releases of the NSPR project.
https://hg.mozilla.org/projects/nspr
Is memset(0) really the correct fix for this bug?

struct tm has a member tm_mday where 0 is an illegal value.

Will a value of zero for all members of this struct still achieve that the function works correctly?

If you wanted to prevent that incorrect data is used, wouldn't it be best to check if the result of MT_safe_localtime is NULL, which indicates that an error has occurred?

After the call, the current implementation assumes that it has obtained the values for Jan 2 1970. I haven't read the code, but I'd assume, the logic implemented in this function will fail, if the exploded values are all zero.

The function should be changed to produce a safe result, if the call to localtime has failed, such as assuming a zero offset from GMT, and filling the output result accordingly.
Attachment #8789706 - Flags: review?(kaie)
Comment on attachment 8789706 [details]
Bug 1227497 - added  memset on localtime.

https://reviewboard.mozilla.org/r/77840/#review86068

::: nsprpub/pr/src/misc/prtime.c:612
(Diff revision 1)
>  
>      secs = 86400L;
> +
> +    memset(&localTime, 0, sizeof(struct tm));
> +
>      (void) MT_safe_localtime(&secs, &localTime);

The correct fix is to check the return value of
MT_safe_localtime().
I see PR_LocalTimeParameters cannot return a failure status.
I am afraid that it will need to call abort() if MT_safe_localtime()
fails.
Thanks for guiding me on this, will repudiate the patch.
Posted patch Modernize MT_safe_localtime (obsolete) — Splinter Review
MT_safe_localtime was written almost 20 years ago. I just
checked the current status.

1. The current version of Single Unix Specification still says
"The localtime() function need not be thread-safe." but we should
be able to assume localtime_r() is available on Unix.

http://pubs.opengroup.org/onlinepubs/9699919799/functions/localtime.html

Right now we use localtime_r() only if the HAVE_POINTER_LOCALTIME_R
is defined. nspr/configure.in doesn't have a feature test for localtime_r().
We should add one. Until then, we need to manually define
HAVE_POINTER_LOCALTIME_R for each Unix platform. In this patch I added
it to Linux and Mac OS X.

2. Visual C++ has had localtime_s() since Visual C++ 2005:
https://msdn.microsoft.com/en-us/library/a442x3ye(v=vs.80).aspx

In this patch I also added a definition of MT_safe_localtime()
based on localtime_s().

Note that localtime() also became thread-safe in Visual C++ 2005:

    Both the 32-bit and 64-bit versions of gmtime, mktime,
    mkgmtime, and localtime all use a single tm structure
    per thread for the conversion. Each call to one of
    these routines destroys the result of the previous call.

https://msdn.microsoft.com/en-us/library/bf12f0hc(v=vs.80).aspx

But it's better to use localtime_s().
Attachment #8802700 - Flags: superreview?(kaie)
Attachment #8802700 - Flags: review?(ted)
The previous patch passes the arguments to localtime_s() in the
wrong order. This patch corrects that.
Attachment #8802700 - Attachment is obsolete: true
Attachment #8802700 - Flags: superreview?(kaie)
Attachment #8802700 - Flags: review?(ted)
Attachment #8802702 - Flags: superreview?(kaie)
Attachment #8802702 - Flags: review?(ted)
Assignee: bpostelnicu → wtc
Comment on attachment 8802702 [details] [diff] [review]
Modernize MT_safe_localtime, v2

Wan-Teh, your explanation makes sense, and the patch looks good to me, thanks for your work, r=kaie
Attachment #8802702 - Flags: superreview?(kaie) → superreview+
If I understand correctly, Wan-Teh's patch is general improvement for NSPR, but it doesn't implement a fix for this bug.

The function might still fail, and if you want to make things work correctly, you must still check if function MT_safe_localtime returns a failure, or not. See comment 8.
(In reply to Wan-Teh Chang from comment #8)
> I see PR_LocalTimeParameters cannot return a failure status.
> I am afraid that it will need to call abort() if MT_safe_localtime()
> fails.

Wan-Teh, I'm worried that introducing a new abort() might be too rough.

Given that today we might potentially use garbage numbers in the worst case, and nobody has ever complained about this behavior yet, I'm not sure that introducing a hard runtime failure is necessary.

If we cannot figure out what localtime is, wouldn't it be sufficient to simply return zero parameters, just like we do for GMT ?
Attachment #8789706 - Attachment is obsolete: true
Attachment #8809154 - Flags: review?(wtc)
Comment on attachment 8802702 [details] [diff] [review]
Modernize MT_safe_localtime, v2

Patch checked in on the NSPR trunk:
https://hg.mozilla.org/projects/nspr/rev/1f5630faef26ec49b472006f20dd35bd5957c211

I forgot to include the bug number in the hg commit message. Sorry!
Attachment #8802702 - Flags: checked-in+
Comment on attachment 8809154 [details] [diff] [review]
Check for failing MT_safe_localtime, patch v3

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

Kai: thank you for the patch. You're right. My patch (attachment 8802702 [details] [diff] [review])
is not a fix for this bug. It is just a general improvement.

I suggest two changes to your patch.

::: pr/src/misc/prtime.c
@@ +605,5 @@
>       *    since Jan. 2, 1970.
>       */
>  
>      secs = 86400L;
> +    if (MT_safe_localtime(&secs, &localTime) == NULL) {

It is important to fail here, because this should not happen.

We are calling this function with input arguments we control,
so it should not fail. If it fails, we need to know and fix
our code. Please use this code:

    if (MT_safe_localtime(&secs, &localTime) == NULL) {
        PR_Assert("MT_safe_localtime(&secs, &localTime) != NULL", __FILE__, __LINE__);
    }

@@ +1624,5 @@
> +                    /* We don't return a failure, because past versions of NSPR
> +                     * didn't check for a failure, and might have used random
> +                     * values. To avoid breaking functionality in an NSPR
> +                     * upgrade, let's simply use a zero offset fallback. */
> +                    zone_offset = 0;

PR_ParseTimeStringToExplodedTime returns PRStatus, so we should fail with PR_FAILURE here.
Attachment #8809154 - Flags: review?(wtc) → review-
(In reply to Wan-Teh Chang from comment #17)
> >  
> >      secs = 86400L;
> > +    if (MT_safe_localtime(&secs, &localTime) == NULL) {
> 
> It is important to fail here, because this should not happen.
> 
> We are calling this function with input arguments we control,
> so it should not fail. If it fails, we need to know and fix
> our code.


We may not know about all circumstances on all platforms that could cause the function to return a failure, and that were previously hidden.

How about only aborting in DEBUG builds, and using the safe fallback in optimized builds?

If we abort in debug mode, only, then we can detect errors made by programmers, but will not make it worse if it's impossible to convert to a local timezone in some environments.


> @@ +1624,5 @@
> > +                    /* We don't return a failure, because past versions of NSPR
> > +                     * didn't check for a failure, and might have used random
> > +                     * values. To avoid breaking functionality in an NSPR
> > +                     * upgrade, let's simply use a zero offset fallback. */
> > +                    zone_offset = 0;
> 
> PR_ParseTimeStringToExplodedTime returns PRStatus, so we should fail with
> PR_FAILURE here.


Aren't you worried that applications might stop working after they upgrade to a newer NSPR version? If this only happens in some special environments, and if we don't abort, it might be difficult to identify this change as the cause.

Since you assume that it won't happen, I suggest we assert/abort in debug builds, and return a failure in optimized builds. That can at least give developers the chance to more easily find this new failure, when they try to debug.

I'm attaching a patch that implements my above suggestion.
Attachment #8809154 - Attachment is obsolete: true
Attachment #8810819 - Flags: review?(wtc)
Comment on attachment 8810819 [details] [diff] [review]
Check for failing MT_safe_localtime, patch v4

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

r=wtc.

Kai, I'm terribly sorry that I forgot to mark this patch
reviewed when I reviewed it.

I wanted to point out that we should not assume people
may run our debug builds. The debug builds of some of
the most important NSPR-based products (such as Firefox)
are too slow to be practical. This is why I no longer
rely on using PR_ASSERT to detect an error.

The problem with returning PR_GMTParameters is that it
is off by hours, which may cause more serious trouble
than aborting the program. This is why I still recommend
aborting the program, even in optimized builds. I
am no longer an NSPR module owner, so I leave this
decision up to you.
Attachment #8810819 - Flags: review?(wtc) → review+
Regarding debug builds:
- developers at least run debug builds when they test their new code
- the firefox automated tests are executed in both optimized and debug builds.

If there is a frequent occurrence of the failure situation, it should be noticed, at least as test failures in the automated tests.


Regarding your worry that returning PR_GMTParameters from function PR_LocalTimeParameters can cause trouble, because it might be off by several hours:

We'll only return that value, if the call to localtime* returns a failure and we're unable to get the local time difference. In this scenario, in the past, the code returned the random contents of an uninitialized value. If this was really happening, it was already much worse. If software is being used despite this incorrect use, being off by a few hours should be less problematic than being off by a random number of days/months/years.

My points are:
- we're not making it worse
- we don't introduce a new crash, but we are handling it leniently, in a less random way then before
- we make it possible it detect the failure in debug builds.

In my opinion this is an improvement. Thank you for the r+, so I'll check this in.

It might be useful to get an additional opinion here, so if someone is reading this bug and has additional thoughts, please speak up.
https://hg.mozilla.org/projects/nspr/rev/bdc2f42edf03
Assignee: wtc → kaie
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 4.14
Attachment #8802702 - Flags: review?(ted)
Target Milestone: 4.14 → 4.15
You need to log in before you can comment on or make changes to this bug.