[Pkg-gnutls-maint] Bug#373169: fixed in 2.3.10

[Simon Josefsson]

Daniel Kahn Gillmor <dkg at fifthhorseman.net> writes:

> On Mon 2008-05-19 14:13:34 -0400, Simon Josefsson wrote:
>
>> Hi!  Thanks for security review of this.  v2.3.10 now uses umask
>> directly, so I think this should be solved:
>>
>> http://git.savannah.gnu.org/gitweb/?p=gnutls.git;a=commitdiff;h=d0629b2c7d20c32c746cc221e997485d63ee2d71
>
> Thanks for the fix, Simon.  That changelog is really noisy because of
> all the comments bouncing.  Are src/certtool-gaa.{c,h} files that are
> that's auto-generated from certtool.gaa?

Yes.

> If so, shouldn't they be removed from the repository as generated
> files, and new rules added to the Make subsystem to create them?
>
> That would make changesets like this one easier to interpret.

Yes, but would also requires everyone building from source to install
GAA which isn't widely available (not packaged for debian for example).
These files change quite infrequently, so my gut feeling is that the
cost of having rare unreadable patches is lower than having a lot of
people having to install GAA manually.  Nikos, what do you think?

> Also, is there a reason to not reset the umask after file creation?
> If certtool goes on to create another file, wouldn't it make more
> sense to leave the umask as you found it?  (e.g. if you were emitting
> the certificate into a different file than the key, you'd probably
> expect the key to be private, but the cert to be publicly readable)

Certtool only opens the --outfile file for writing, so there is only one
file to consider.  However, maybe at some point certtool will write more
than one file.  Is the following a good idea?

diff --git a/src/certtool.c b/src/certtool.c
index 1463a1f..bf0990f 100644
--- a/src/certtool.c
+++ b/src/certtool.c
@@ -821,14 +821,22 @@ update_signed_certificate (void)
 
 FILE* safe_open_rw(const char* file)
 {
-#ifdef HAVE_UMASK
-      if (info.privkey_op != 0) 
-        {
-	  umask(S_IRGRP|S_IWGRP|S_IROTH|S_IWOTH);
-        }
-#endif
+  mode_t oldmask;
+  FILE *fh;
+
+  if (info.privkey_op != 0)
+    {
+      oldmask = umask (S_IRGRP|S_IWGRP|S_IROTH|S_IWOTH);
+    }
+
+  fh = fopen (file, "wb");
+
+  if (info.privkey_op != 0)
+    {
+      umask (oldmask);
+    }
 
-      return fopen (file, "wb");
+  return fh;
 }
 
 void

/Simon