832932 - NSPR: stricter bound check for prtime.c PR_ParseTimeString and friends.

Status: ASSIGNED

(NSPR :: NSPR, defect)

Description

[ISHIKAWA, Chiaki]

Attachment: A work-in-progress patch for prtime.c

During running the debug build version of TB under valgrind (memcheck),
it was noticed that PR_ParseTimeStringToExplodedTime() was sometimes
scanning the passed buffer well beyond the valid memory range and caused
memcheck to flag the operation as invalid memory access.

After checking the code, it was found that the upperlimit of 1000 is not
quite observed within the subloop in the PR_ParseTimeStringToExplodedTime().

So the stricter checking is inserted.

(The original cause of the problem may be related to the passing of bogus
memory area due to possible memory corruption: 
See Bug 814870 - Invalid read of size 1 in PR_ParseTimeStringToExplodedTime (prtime.c:1450) )

Proposed patch attached.
 
(I have a more detailed patch to clean up some hard to read code, addition of CET (Central European Time) and such,
but I only have the bound-check and removal of trailing blanks in this patch.)

Comment 1

[ISHIKAWA, Chiaki]

Hi,

the patch is being tested locally and on the tryserver.
As far as I can tell, it works as expected.

One nit, though. I thought I would try checking Win build on the tryserver, but
obviously something is very wrong with my setup. It builds clean on TryServer for linux32, and linux64, but for windows build, the patch fails in the first place.
From the manner that every single hunk fails I suspect a line terminator (LF vs CRLF) mismatch. 

But that sounds so strange. 
Does people keep two separate copies of source tree for
LF (for linux, say) vs CRLF files (for windows)?
If so, I am not ready to test win build.

But I thought reporting that linux32, and linux64 worked as expected.

Comment 2

[[e-mail]]

(In reply to ISHIKAWA, chiaki from comment #1)
> One nit, though. I thought I would try checking Win build on the tryserver,
> but obviously something is very wrong with my setup. It builds clean on
> TryServer for linux32, and linux64, but for windows build, the patch fails
> in the first place.
> From the manner that every single hunk fails I suspect a line terminator (LF
> vs CRLF) mismatch. 
I've heard that there's a bug in comm-central's Try server, if that's what you were using. Apparently you can work around it by removing the ALWAYS_RUN_CLIENT_PY lines from all of the config/mozconfigs/win32/* and replace the --hg-tool=<path> with --apply-patches (which does not take a path) in the build/client.py-args file.

Comment 3

[ISHIKAWA, Chiaki]

(In reply to [e-mail] from comment #2)
> (In reply to ISHIKAWA, chiaki from comment #1)
> > One nit, though. I thought I would try checking Win build on the tryserver,
> > but obviously something is very wrong with my setup. It builds clean on
> > TryServer for linux32, and linux64, but for windows build, the patch fails
> > in the first place.
> > From the manner that every single hunk fails I suspect a line terminator (LF
> > vs CRLF) mismatch. 
> I've heard that there's a bug in comm-central's Try server, if that's what
> you were using. Apparently you can work around it by removing the
> ALWAYS_RUN_CLIENT_PY lines from all of the config/mozconfigs/win32/* and
> replace the --hg-tool=<path> with --apply-patches (which does not take a
> path) in the build/client.py-args file.

Yes, I have used comm-central's Try Server.
I had this "--apply-paches" line (the second half of your suggestion.)

But I did not remove "ALWAYS_RUN_CLIENT_PY" lines from the config/mozconifgs/win32/* (the first half of your suggestion.)
I will investigate.

TIA

Comment 4

[ISHIKAWA, Chiaki]

I think it is about time that this patch go in the tree.
I have been using this locally for a long time.
(Maybe I may have to upload the recent version to make sure the patch applies cleanly.)
I thought that there was an issue of pending patch or something (or some other patches that
may need baking so to speak: see
Bug 814870 - Invalid read of size 1 in PR_ParseTimeStringToExplodedTime (prtime.c:1450)
)
But it has been a year, and so I think I will upload a new patch very shortly to make sure it applies cleanly
and hope it gets the approval for inclusion soon.

TIA

Comment 5

[ISHIKAWA, Chiaki]

I will upload a new patch just to make sure that bitrot issue is taken care of.

Comment 6

[ISHIKAWA, Chiaki]

Attachment: NSPR: stricter bound check for prtime.c PR_ParseTimeString and friends.

This is an updated patch, and lands cleanly.
I modified the definition of check macro by using |do { ... } while (0)| 
instead of |if (1) { ... } else| since the latter causes GCC compiler warning about surrounding the statement after |else|, etc.

TIA

Comment 7

[ISHIKAWA, Chiaki]

I was not sure if [e-mail] is still around, and so asked ted to review this.
TIA

Comment 8

[ISHIKAWA, Chiaki]

I think [e-mail] is still around and so request information.

Comment 9

[(not currently active) Ted Mielczarek]

Comment on attachment 8796834 [details] [diff] [review]
NSPR: stricter bound check for prtime.c PR_ParseTimeString and friends.

I have resigned as NSPR module owner. Sorry for the inconvenience.

Comment 10

[Magnus Melin [:mkmelin]]

Chiaki, how about instead advancing rest through a method which would check limits before advancing?

Also, to ease review, drop all the unrelated whitespace changes and such (can be submitted separately), and remove the debugging code like dumpstring.

After updating the patch ask for review - https://wiki.mozilla.org/Modules/All#NSPR

Comment 11

[ISHIKAWA, Chiaki]

(In reply to Magnus Melin [:mkmelin] from comment #10)

Chiaki, how about instead advancing rest through a method which would check
limits before advancing?

Also, to ease review, drop all the unrelated whitespace changes and such
(can be submitted separately), and remove the debugging code like dumpstring.

After updating the patch ask for review -
https://wiki.mozilla.org/Modules/All#NSPR

I must have missed this comment (sorry, my e-mail box swamped about 7 months ago.)

I will try to follow the advice.

Comment 12

[ISHIKAWA, Chiaki]

Sorry, I have been side-tracked by heavy workload of my day job. But I have started to work on this.
Stay tuned.

Comment 13

[ISHIKAWA, Chiaki]

Hi, I am creating a patch for mozilla/nsprpub/pr/src/misc/prtime.c.

I am requesting whether I should use clang-format pretty printer or not.

Background:

I thought every source file in mozilla-central has been run through clang-format.
But maybe the file under pr was not formatted according to clang-format.
I found out this out when I try to use ./mach clang-format to pretty print my change against the latest source I fetched a few days ago.

So I suspect that my patch should not try to be created against the reformatted according to clang-format.

Lately, there has been a talk of installing a hook for automatic clang-format a submitted patch.
I think when I ran "./mach bootstrap", it offered to install a local hook to run clang-format even.
So I am a bit confused whether I should create a patch using ./mach clang-format locally
(which unfortunately created a lot of whitespace changes and not so nice indentation changes for unfathomable reason.).

A guidance as to whether I should NOT use clang-format is appreciated.
If given a choice, I would NOT use clang-format due to the strange formatting (indentation is screwed for some unfathomable reason and create so many whitespace changes in live code sections. The result is a patch very hard to review.
Harder than the old patch in this bugzilla which did have a few white space changes. And for that matter, ./mach clang-format does not seem to delete trailing whitespace, which I wish it would).

TIA

Comment 14

[ISHIKAWA, Chiaki]

(In reply to Magnus Melin [:mkmelin] from comment #10)

Chiaki, how about instead advancing rest through a method which would check
limits before advancing?

Also, to ease review, drop all the unrelated whitespace changes and such
(can be submitted separately), and remove the debugging code like dumpstring.

After updating the patch ask for review -
https://wiki.mozilla.org/Modules/All#NSPR

Hi, I have removed all the whitespace changes from my original patch.

I have a question about this part:

advancing rest through a method which would check
limits before advancing

Do you have in mind something like, say, a MACRO, that looks like this?

The original piece of code with missing checks:

                       rest++; /* move over sign */
                       end = rest;
                        while (*end >= '0' && *end <= '9')
                          end++;

My original patch:

                         rest++; /* move over sign */
                         end = rest;
                         BOUNDCHECK(end - string); /* CAUTION: out of array boundary  */
                         while (*end >= '0' && *end <= '9')
                         {
                           end++;
                           BOUNDCHECK(end - string);
                         }

Would the following be what you possibly have in mind?
SAFEBUMP(p) increments the pointer p and then if there is a possible overflow, the function returns.
Since it needs to change the pointer AND we need to return from the function, I prefer to use a MACRO here.

                        SAFEBUMP(rest) /* move over sign */
                         end = rest;
                         while (*end >= '0' && *end <= '9')
                         {
                           SAFEBUMP(end);
                         }

Maybe SAFEBUMP is too wordy and BUMP is preferable, though.

TIA

Comment 15

[ISHIKAWA, Chiaki]

I hasten to add that there are idioms in the original code where pointer++ operation is used
in a manner that do not mesh well with SAFEBUMP(pointer) macro. So maybe we can reduce the usage of BOUNDCHECK() by half, but there are situations that need explicit checking by BOUNDCHECK() after all.

Comment 16

[Magnus Melin [:mkmelin]]

(In reply to ISHIKAWA, Chiaki from comment #14)
Yeah something like that macro should be ok I think.

Sorry but NSPR is special. I don't know if they run the mach clang-format for that. (Would be great if they did.)

Comment 17

[Kai Engert (:KaiE:)]

Hello Chiaki, I think there might be a confusion about the existing code.

The variable "string" points to the beginning of the input, and is never modified.

The function walks along the input string, looking what's inside it. It uses the pointer variable "rest", which will point to a location inside the input "string".

If the function looks at substrings, the function uses additional helper variables like "s" and "end", that will point to a location that is after "rest".

You use the macro BOUNDCHECK to calculate the difference between the current location (rest, s, end) and the beginning of the string.

With the macro BOUNDCHECK you suggest, that the distance from the beginning of the string must be at most 1000 bytes.

I think you have misunderstood the meaning of the number 1000.

I think the input string to the function can be any size. The string ends when the byte at the location has value zero, which is commonly used to mark the end of a string.

You can see that this is the expectation of the function, because it uses expressions like "while (*rest)", and also later it checks for "*rest", which means, it checks if *rest is different from the null terminator byte.

The number 1000 as used in the original code is for a different purpose. Apparently the intention of that limit 1000 is:
"If the code has walked through the loop 1000 times, and we still haven't found a final result, then probably something is wrong with the logic, probably the string doesn't match the expectation, and then the code gives up and returns a failure."

In other words, I believe your BOUNDCHECK doesn't help. The input string could be smaller than 1000 characters, and then your checks never have any effect.

If you can show that I'm wrong, please do. Maybe I misunderstood.

Why did valgrind see an "invalid memory access"?
I think I found a problem.

The code checks that *rest isn't zero. But then it looks at the characters that follow, like rest[1], rest[2], rest[3]. But rest[1] could already be a zero byte, then rest[2] is an invalid access.

The code needs to be enhanced to check how many bytes are remaining. To do that, at the beginning of the function, the code should call "strlen" to obtain the length of the input. Then it should calculate a "end_of_string" char pointer, that points to the end of the string. Whenever the code wants to "look ahead" (e.g. rest[1], rest[2]), it must check that location "rest" plus the number of bytes the code wants to look ahead is still below the end_of_string.

Comment 18

[ISHIKAWA, Chiaki]

(In reply to Kai Engert (:kaie:) from comment #17)

Hello Chiaki, I think there might be a confusion about the existing code.

Let me digest your comment.

There was a time when TB called this with a bogus pointer due to an error in its own code and as a result, the illegal memory access occurred.

You are right that 1000 is a ball park figure for a reasonable limit of Date string often found in header line of e-mail message or response to
an HTTP request.

To be honest, I don't think the call of strlen() at the beginning is a great idea. This is because
the routine seems to be passed a user-supplied string. We may want to use strnlen() instead with the maximum of, say, 1000.

The problem, of course, is that the routine does not receive the length of the buffer that we should use as the upper limit.

Let me think of a modification then.

BTW, what about the clang-format pretty-printing / reformatting of the code?

TIA

Chiaki

Comment 19

[Kai Engert (:KaiE:)]

Can you find an example input that resulted in a crash?

I haven't research who calls the function, and where the input originates. The NSPR tests only use input strings that NSPR created by itself.

I'll attach a demo patch, that demonstrates the invalid access, even if the input string is simply "a".

Regarding reformat of the code, I think it's very confusing to review patches that also contain white space changes. I don't mind starting a project to reformat all of NSPR. However, for all other bugs, I think whitespace shouldn't be touched, to simplify reviewing.

Comment 20

[Kai Engert (:KaiE:)]

Attachment: 832932-demo.patch

Demonstration patch. With this patch applied, build NSPR, then also build inside the nspr/{objdir}/pr/tests directory (make). Then execute ./timetest -d
Crashes immediately because of the rest[1] and rest[2] code, which is detected by the assert in the helper function.

Comment 21

[Kai Engert (:KaiE:)]

I would like to see a list of strings that is passed to this function during a typical Thunderbird session.

Comment 22

[Kai Engert (:KaiE:)]

Attachment: 832932-demo2.patch

This is the kind of check that the code should do.

Whenever it wants to "look ahead" and access rest[1] and rest[2] (in addition to the third byte rest[0]), it should ensure that there are indeed at least three bytes remaining, starting at rest.

E.g. using ENSURE_REMAINING(3)

Comment 23

[Kai Engert (:KaiE:)]

Calling strnlen to determine an end position doesn't help, if we continue to look for the end-of-string by (*rest) != 0.

The existing maximum iteration count 1000 already ensures a limit on how much of the input will be processed.

Comment 24

[Kai Engert (:KaiE:)]

(In reply to ISHIKAWA, Chiaki from comment #18)

BTW, what about the clang-format pretty-printing / reformatting of the code?

Let's handle that separately in bug 1375876

Comment 25

[ISHIKAWA, Chiaki]

(In reply to Kai Engert (:kaie:) from comment #21)

I would like to see a list of strings that is passed to this function during a typical Thunderbird session.

I would report on this and other issues once I get the latest C-C code, fetched last Friday or so, to compile.
https://bugzilla.mozilla.org/show_bug.cgi?id=1562504

TIA

Comment 26

[ISHIKAWA, Chiaki]

I am afraid this dropped through the fingers, so to speak, during Covid-19 outbreak work style change.
Ping myself again.