835029 - [patch] Readlink - off-by-one

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Maxime Villard]

Attachment: firefox.diff

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:18.0) Gecko/20100101 Firefox/18.0
Build ID: 20130109165059

Steps to reproduce:

Looked at the code


Actual results:

Found mistakes


Expected results:

The readlink() function is sometimes misused, see this:
https://www.securecoding.cert.org/confluence/display/seccode/POS30-C.+Use+the+readlink%28%29+function+properly

Patch attached.

Comment 1

[Matthias Versen [:Matti]]

A patch in 2 different areas of the code can complicate the review but thanks for the patch !
You should also request a review from someone or the patch could be overseen

Comment 2

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 706768 [details] [diff] [review]
firefox.diff

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

We will have to check the security/coreconf fix into NSS separately from the toolkit/ fix.

::: security/coreconf/nsinstall/pathsub.c
@@ +240,4 @@
>  	if (rv < 0) {
>  	    perror(myPath);
>  	} else if (S_ISLNK(sb.st_mode)) {
> +	    rv = readlink(myPath, buf, sizeof buf-1);

Please put spaces around "-", i.e. "sizeof buf - 1"

::: toolkit/crashreporter/google-breakpad/src/client/solaris/handler/minidump_generator.cc
@@ +511,4 @@
>    int count;
>  
>    snprintf(path, sizeof (path), "/proc/self/path/%s", module_info.name);
> +  if ((count = readlink(path, buf, PATH_MAX-1)) < 0)

ditto: "PATH_MAX - 1"

Comment 3

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Reminder to self that I need to check in the coreconf fix once the NSS tree re-opens.

Comment 4

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: bug-835029.patch

I split the NSS part off into bug 938486.

Will check this in when mozilla-inbound opens.

Comment 5

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Actually, checkin-needed is magic now, so trying that.

Comment 6

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

clearing needingo

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8faa1855c483

Comment 8

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/8faa1855c483