In bug 463452, the file creation of NSS files were changed to always use the 0600 permissions on Unix, which restricts reading and writing to the owner of the file. I believe the original intention was to protect the cert and key databases, however, the list of registered pkcs#11 modules should be less problematic, in my opinion. On Linux, when using a systemwide NSS database for default settings (nss-sysinit related), this is problematic, as the list of registered modules should be accessible to any user. I found that lib/util/utilmod.c is nowadays used for opening the pkcs11.txt file, function lfopen() uses open(,,0600); It seems function lfopen is called from inside that file, only, and only from the two functions: * nssutil_DeleteSecmodDB * nssutil_AddSecmodDB If it's true that nobody else calls lfopen, then I propose to remove lfopen, and to replace calls to it using fopen. When overwriting existing files with "w+"/truncate it will keep the file permissions - or use the default umask. I assume the creation of key/cert database is handled elsewhere, as there aren't callers to lfopen from outside of utilmod.c, therefore the proposed change shouldn't cause regressions for the key/cert database files.

Basically I'm reverting half of the original change (the part that deals with pkcs11.txt), while keeping the changes for the other files. (See bug 463452 attachment 351481 [details] [diff] [review] for the original change).

See my comment on the original bug. I don't think we should monkey with the default permissions of pkcs11.txt. bob

Bob, which comments are you referring to? That said, my initial reaction to this was to think that this would be 'dangerous', in the event of something 'hostile' replacing the PKCS#11 modules to be loaded, so that keys are stored/unwrapped in a different module than the user intended. Perhaps this isn't possible, but I'm nervous whenever proposing to reduce the strictness of a security check, and think the burden should be on why it's safe.

There was a misunderstanding that we clarified during a phone call. The problem is that core NSS *changes* the permissions of the existing pkcs11.txt file, *reverting* them back to the default 0600, even if the file previously had other permissions. We agreed on the following during the call: - if pkcs11.txt does not yet exist, we should create it as 0600 - if pkcs11.txt already exists, we must keep the permissions when modifying As of today, our code uses a series of two commands, in order to use the permission flags, but still associate it with a FILE*: - fd = open(name, O_CREAT|O_RDWR|O_TRUNC, 0600); - fdopen(fd, "w+"); It's more complicated even, the existing code in nssutil_DeleteSecmodDB might sometimes erase the old file and create a new one. I conlcude: The correct implementation strategy is to use "stat" to obtain the existing permissions, and using them when creating the new files.

I'm adjusting the subject based on the latest agreement. We'll keep the strict default 0600 for new files, but we'll keep the existing permissions when replacing the existing pkcs11.txt file. Regarding Ryan's comments, I could becalm his worries by explaning that this is only about the pkcs11.txt file, not about the other ones. And given we're keeping the strict default, I assume the worries are settled.

Thanks for the clarification Kai. I now agree this is a legitimate bug, I only have qualms about the original proposed fix. I'll review your new patch shortly. bob

Comment on attachment 784477 [details] [diff] [review] patch v2 Review of attachment 784477 [details] [diff] [review]: ----------------------------------------------------------------- ::: lib/util/utilmod.c @@ +142,5 @@ > #include <unistd.h> > #endif > #include <fcntl.h> > > /* same as fopen, except it doesn't use umask, but explicit */ This comment is no longer correct because lfopen now has an additional |file_mode| argument. Also note that fopen has only two arguments: http://pubs.opengroup.org/onlinepubs/009695399/functions/fopen.html So the |flags| argument should also be documented. It is a little confusing to have both |mode| and |file_mode|, so at least the extra |file_mode| argument should be documented. It may be good to rename |mode| to |open_mode|. @@ +144,5 @@ > #include <fcntl.h> > > /* same as fopen, except it doesn't use umask, but explicit */ > FILE * > +lfopen(const char *name, const char *mode, int flags, mode_t file_mode) Please check whether the mode_t type exists on Windows: http://msdn.microsoft.com/en-us/library/z0kc8e3z(v=vs.110).aspx If not, we may need to use 'int' instead. @@ +480,3 @@ > /* do we really want to use streams here */ > fd = fopen(dbname, "r"); > if (fd == NULL) goto loser; Nit: in general, using fstat() on an open fd is more secure than using stat() on a file name. It may not matter in this case, but since we can easily switch to fstat(), I suggest we do that. /* do we really want to use streams here */ fd = fopen(dbname, "r"); if (fd == NULL) goto loser; + /* get the permissions of the existing file, or use 0600 */ + if (!fstat(fd, &stat_existing)) { + file_mode = stat_existing.st_mode; + } else { + file_mode = 0600; + } @@ +608,5 @@ > + } else { > + file_mode = 0600; > + } > + > + fd = lfopen(dbname, "a+", O_CREAT|O_RDWR|O_APPEND, file_mode); This case requires more thought. First, I think we need to get the file mode of the existing file |dbname| before we delete it with nssutil_DeleteSecmodDB. Second (optional): I think we can just open the existing file with the "truncate" flag/mode, which will effectively delete the previous version.

(In reply to Kai Engert (:kaie) from comment #0) > > If it's true that nobody else calls lfopen, then I propose to remove lfopen, > and to replace calls to it using fopen. When overwriting existing files with > "w+"/truncate it will keep the file permissions - or use the default umask. Good, so you already know that we can open an existing file with the "truncate" flag/mode, which will keep the file permissions. The mode_t file_mode argument is only used when open() creates a file, otherwise it is ignored. Also, I have to admit I don't understand the "except it doesn't use umask" comment in the code, because I think open() honors umask.

> Second (optional): I think we can just open the existing file with the > "truncate" flag/mode, which will effectively delete the previous version. The "use a second file" was done on purpose. If we experience a crash, or someone opens the file at the wrong moment, we don't get corrupted data. bob

Bob: I suspect you misunderstood the context for that comment of mine. That comment of mine is for the nssutil_AddSecmodDB function. It is better to view my comments in the Splinter Review tool. Thanks.

review ping, this is an important correctness fix.

Comment on attachment 784477 [details] [diff] [review] patch v2 r+ rrelyea

> That comment of mine is for the nssutil_AddSecmodDB function. So I'm confused why you want to truncate when we can clearly append. If we truncate on Add, then we would again need to have a second file (like delete) to prevent the same bad issues from occuring. > It is better to view my comments in the Splinter Review tool. Thanks. comments in an external tools should have links in this bug if they need be considered. thanks, bob

re Splinter review. my bad, it is part of the bugzilla tools. Looking at that, my preference for the current method stands (O_TRUNC actually won't work because we'd looks all the existing context. We actually need to append). bob

Thanks for your comments. An important detail, which I had missed earlier, is Wan-Teh's reminder to ensure compatibility with Windows - which requires me to add platform specific code! But let me address some comments in order: (a) Wan-Teh's "nit" comment for the "Delete" function. I agree that using "fstat" instead of "stat" is a slight improvement, and because it's a trivial change, I'll apply it. It involves moving the stat block to after opening the first file. (b) Regarding this existing comment /* same as fopen, except it doesn't use umask, but explicit */ Wan-Teh said: "This comment is no longer correct because lfopen now has an additional |file_mode| argument." Do you complain that standard function fopen has two parameters, but our lfopen has one more parameter? Well, that's not the only difference, because our lfopen now has even 4 params. It hasn't been "the same", and now it will be "even less the same", because it now has 4 params :) It might make more sense to provide a high level comment for the function. The intention of the function seems to be: "Open a file descriptor with the full flexibility that the "open" function offers, as specified by the "flags" and "file_mode" parameters. Then transform that file descriptor into a buffered FILE* structure. It's required to provide an fopen-style mode string that is compatible with the "flags" parameter." But then I realized, things will become more complicated, because of (c). Read on. (c) Wan-Teh's reminder that file_mode might not work on Windows. Thanks for the comment, I had missed it previously. Function "open" is deprecated, and we should rather use "_open". The constants for the flag parameter are different, they use an additional initial underscore. The third parameter must be provided in case we use the CREAT flag opening. This means, we need some platform specific code... fdstat and fdopen are deprecated, too, have _fdstat and _fdopen replacements. I assume that permission file_mode parameters like "0600" don't work on Windows. Instead, there is a choice to use read-allowed and write-allowed flags. I propose that on Windows, we keep the st_mode flags we obtain from fstat, and by default use the read/write flags. Because we'll need some special handling, I'll rather take away the full flexibility of the lfopen parameter list, and restrict it to the two cases that we need, truncate and append... This eliminates the need to fully document the parameters. (d) Wan-Teh's "optional" proposal to "just truncate" in the add function. Both Bob and I don't like this suggestion, so I'll skip it. (e) Wan-Teh's comment that the change to the AddSecmodDB function needs more thought. He suggest to get the file mode prior to deleting it. But we aren't deleting the file! The call to DeleteSecmodDB will simply remove an entry from the file, by replacing it with an updated file, which will also keep the existing permissions. So, the code in AddSecmodDB to obtain the file mode will work. (f) I think the current naming of the functions is slightly confusing, because we aren't adding/deleting a DB, but rather, we are adding/deleting ENTRIES from a DB. Since the functions are static, I'll rename them to include the word "entry".

I believe the changes I made were straightforward and addressed the existing review comments. So, it might not be necessary to do a full re-review. I suggest that I go ahead and check it in, and see if it builds successfully on Windows. But you're welcome to re-review.

Comment on attachment 8394890 [details] [diff] [review] p6-900067.patch Review of attachment 8394890 [details] [diff] [review]: ----------------------------------------------------------------- ::: lib/util/utilmod.c @@ +25,3 @@ > #if defined (_WIN32) > #include <io.h> > +/* pf means "platform" */ I suggest you use "os" instead of "pf". "OS" is a well-known abbreviation and means essentially the same as "platform". Then you don't need to document what the os_ prefix means. @@ +154,5 @@ > } > > +static SECStatus nssutil_AddSecmodDBEntry(const char *appName, > + const char *filename, > + const char *dbname, Nit: since you are modifying these lines, could you delete the space characters at the end of the lines? @@ +465,5 @@ > */ > static SECStatus > +nssutil_DeleteSecmodDBEntry(const char *appName, > + const char *filename, > + const char *dbname, Nit: delete the spaces at the end of lines. @@ +606,5 @@ > * Add a module to the Data base > */ > static SECStatus > +nssutil_AddSecmodDBEntry(const char *appName, > + const char *filename, const char *dbname, Nit: delete the spaces at the end of lines.

Wan-Teh, thanks, I've addressed your comments.

I've checked in p7: https://hg.mozilla.org/projects/nss/rev/aeb5967e0221 This is to ensure that patch works correctly on Windows. I'll make adjustments (or back out) if necessary.

> (a) > Wan-Teh's "nit" comment for the "Delete" function. I agree that using > "fstat" instead of "stat" is a slight improvement, and because it's a > trivial change, I'll apply it. It involves moving the stat block to after > opening the first file. Wan-Teh, both of us didn't notice that "fd" is of type FILE*, but fstat takes argument int. I changed the code back to my original proposal, to use "stat" with the filename, prior to opening the file. Both places in the code use the same approach.

Two follow up checkins were necessary: - to address comment 21 https://hg.mozilla.org/projects/nss/rev/ff3e94101e22 - adjust #include statements for windows https://hg.mozilla.org/projects/nss/rev/dbb9bec0b6e0 I'm attaching a "combined patch", that is the sum of the three commits.

Kai, you can still use fstat: fstat(fileno(fd), ...). bob