634490 - nss_clean_all fails in Windows 7 without administrator privileges

Status: RESOLVED FIXED

(NSS :: Build, defect, P2)

Description

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

The problem is that the *.chk files are created in such a way that delete permission is not granted to the user. The build script should change so that the user can delete these files without UAC elevation.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

brian, 
any clues about what is being done "wrong" in the creation of the chk files?

Comment 2

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

This occurs with OS_TARGET=WIN95 and OS_TARGET=WINNT.

shlibsign.c uses PR_OpenFile(...,0666) to open/create the .chk file:

> fd = PR_OpenFile(output_file,PR_WRONLY|PR_CREATE_FILE|PR_TRUNCATE,0666);

See Wan-Teh's comment in bug 298528 comment 1:

> There may be undesirable interactions when the
> file is created by PR_OpenFile but its parent
> directory is not created by PR_MakeDir. We ran
> into such a problem before. I seem to remember
> the problem was that we could not delete a file
> created by PR_OpenFile because its parent
> directory (not created by PR_MakeDir) had the
> wrong permissions or didn't have the right
> permissions. So you need to think about the
> "delete" permission and you need to think
> about the permissions of the parent directory
> whenever you use PR_OpenFile.

I wonder why the Windows tinderboxes don't run into this error. Are they running under an administrator account?

Comment 3

[Wan-Teh Chang]

I've known about this bug for a long time.  I don't know why it
hasn't been reported in Bugzilla before.

There are two solutions.

1. Change shlibsign.c to use PR_Open instead of PR_OpenFile.

2. Change PR_OpenFile to add the Windows "DELETE" permission
when the Unix "w" permission is specified.  Although this doesn't
match the Unix "w" specification exactly, it matches what a
native person would expect (if I have "w" permission on a file,
I should be able to delete it).

I am in favor of solution 2, because it will make PR_OpenFile
more usable.  Previously, Thunderbird also changed its PR_OpenFile
call back to PR_Open for this problem (when saving an email
message).

Comment 4

[Wan-Teh Chang]

I had a typo in my previous comment.  "a native person" should be "a naive person".

Comment 5

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

Attachment: Replace all uses of PR_OpenFile within NSS with PR_Open

I think the first option is the best because (a) it is very unusual on Windows to have files (as opposed to folders) with explicit ACLs, (b) many Windows users cannot even inspect the ACLs on files because Windows Home editions do not include the "security" tab in the Explorer UI, and (c) the semantics for "group" permissions seem arbitrary (why only the primary group and not all groups the user belongs to?). 

softoken/legacydb seems to have the "PR_OpenFile in a directory not created by PR_MakeDir" bug.


Experience with PR_OpenFile suggests we may be better off deprecating it because it is error-prone, undocumented, and implements fairly arbitrary semantics.

PR_OpenFile is not used in mozilla-central outside of NSS.

Comment 6

[Wan-Teh Chang]

Comment on attachment 513010 [details] [diff] [review]
Replace all uses of PR_OpenFile within NSS with PR_Open

Thanks for the patch.

The shligsign.c change is good.

The mangle.c change is not necessary.  PR_OpenFile ignores the
'mode' argument unless the PR_CREATE_FILE flag is specified.

Don't change dbmshim.c.  The dbs_writeBlob call is ultimately
used to create certdb's overflow files with 0600 permissions:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/softoken/legacydb/pcertdb.c&rev=1.12&mark=4037#4025

It is important that certdb's overflow files (logically part of
the certdb) be unreadable and unwritable by other users.

Comment 7

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

Attachment: Replace use of PR_OpenFile with PR_Open in shlibsign

Comment 8

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

(In reply to comment #6)
> Don't change dbmshim.c.  The dbs_writeBlob call is ultimately
> used to create certdb's overflow files with 0600 permissions:
> http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/softoken/legacydb/pcertdb.c&rev=1.12&mark=4037#4025
> 
> It is important that certdb's overflow files (logically part of
> the certdb) be unreadable and unwritable by other users.

How does NSS ensure that the main db file is unreadable and unwritable by others? Or, why is this more important for the blob files than it is for the main db file?

Comment 9

[Wan-Teh Chang]

Brian,

You can search for "0600" in the lib/softoken/legacydb directory:
http://mxr.mozilla.org/security/search?string=0600

In short, we call dbopen() with 0600.  dbopen() calls open().
So the 0600 mode is passed to open().  We can fix PR_OpenFile
by studying how open() in the Microsoft CRT implements its
permission mode argument:
http://msdn.microsoft.com/en-us/library/z0kc8e3z(v=VS.100).aspx

Comment 10

[Wan-Teh Chang]

Comment on attachment 513200 [details] [diff] [review]
Replace use of PR_OpenFile with PR_Open in shlibsign

r=wtc.

Comment 11

[Wan-Teh Chang]

Patch checked in on the NSS trunk (NSS 3.13) and
the NSS_3_12_BRANCH (NSS 3.12.10).

Checking in shlibsign.c;
/cvsroot/mozilla/security/nss/cmd/shlibsign/shlibsign.c,v  <--  shlibsign.c
new revision: 1.19; previous revision: 1.18
done

Checking in shlibsign.c;
/cvsroot/mozilla/security/nss/cmd/shlibsign/shlibsign.c,v  <--  shlibsign.c
new revision: 1.18.20.1; previous revision: 1.18
done