Fix crash when an SSL key-log file couldn't be opened. (issue 10636062)

[a...@chromium.org]

Reviewers: Ryan Sleevi,

Description:
Fix crash when an SSL key-log file couldn't be opened.

BUG=none
TEST=`SSLKEYLOGFILE=/ ./out/Release/chrome` shouldn't crash.


Please review this at https://chromiumcodereview.appspot.com/10636062/

SVN Base: svn://svn.chromium.org/chrome/trunk/src

Affected files:
M net/third_party/nss/README.chromium
M net/third_party/nss/patches/applypatches.sh
A net/third_party/nss/patches/sslkeylogerror.patch
M net/third_party/nss/ssl/sslsock.c


Index: net/third_party/nss/README.chromium
diff --git a/net/third_party/nss/README.chromium
b/net/third_party/nss/README.chromium
index
b16bb66d7b5f14ed2bdbceb1e05bb0ba9d945c44..fd4238fdd47cf1fd4c6de3a979b18a068867bb14
100644
--- a/net/third_party/nss/README.chromium
+++ b/net/third_party/nss/README.chromium
@@ -72,6 +72,7 @@ Patches:
* Move SSL keylogging out from behind the TRACE and DEBUG defines and add
support for CLIENT_RANDOM keylogging to support ECDHE-RSA and others.
patches/keylog.patch
+ https://bugzilla.mozilla.org/show_bug.cgi?id=762763

* SSL_GetChannelInfo and SSL_GetNegotiatedHostInfo should use cwSpec
instead of crSpec to support False Start.
@@ -81,6 +82,9 @@ Patches:
* Add support for extracting the tls-unique channel binding value
patches/tlsunique.patch

+ * Don't crash when the SSL keylog file cannot be opened.
+ patches/sslkeylogerror.patch
+
Apply the patches to NSS by running the patches/applypatches.sh script.
Read
the comments at the top of patches/applypatches.sh for instructions.

Index: net/third_party/nss/patches/applypatches.sh
diff --git a/net/third_party/nss/patches/applypatches.sh
b/net/third_party/nss/patches/applypatches.sh
index
d0500c57709706078506603293add38e06fd46dd..b586613eb21b5784957fbc0d5a6c536a2989132b
100755
--- a/net/third_party/nss/patches/applypatches.sh
+++ b/net/third_party/nss/patches/applypatches.sh
@@ -45,3 +45,5 @@ patch -p4 < $patches_dir/keylog.patch
patch -p4 < $patches_dir/getchannelinfo.patch

patch -p4 < $patches_dir/tlsunique.patch
+
+patch -p4 < $patches_dir/sslkeylogerror.patch
Index: net/third_party/nss/patches/sslkeylogerror.patch
diff --git a/net/third_party/nss/patches/sslkeylogerror.patch
b/net/third_party/nss/patches/sslkeylogerror.patch
new file mode 100644
index
0000000000000000000000000000000000000000..597f64e0c4e6c92618301e3d50007083289cc342
--- /dev/null
+++ b/net/third_party/nss/patches/sslkeylogerror.patch
@@ -0,0 +1,24 @@
+diff --git a/net/third_party/nss/ssl/sslsock.c
b/net/third_party/nss/ssl/sslsock.c
+index 1823a1c..7ef1338 100644
+--- a/net/third_party/nss/ssl/sslsock.c
++++ b/net/third_party/nss/ssl/sslsock.c
+@@ -2934,11 +2934,15 @@ ssl_SetDefaultsFromEnvironment(void)
+ ev = getenv("SSLKEYLOGFILE");
+ if (ev && ev[0]) {
+ ssl_keylog_iob = fopen(ev, "a");
+- if (ftell(ssl_keylog_iob) == 0) {
+- fputs("# SSL/TLS secrets log file, generated by NSS\n",
+- ssl_keylog_iob);
++ if (!ssl_keylog_iob) {
++ perror("Failed open key log file");
++ } else {
++ if (ftell(ssl_keylog_iob) == 0) {
++ fputs("# SSL/TLS secrets log file, generated by NSS\n",
++ ssl_keylog_iob);
++ }
++ SSL_TRACE(("SSL: logging pre-master secrets to %s", ev));
+ }
+- SSL_TRACE(("SSL: logging pre-master secrets to %s", ev));
+ }
+ ev = getenv("SSLBYPASS");
+ if (ev && ev[0]) {
Index: net/third_party/nss/ssl/sslsock.c
diff --git a/net/third_party/nss/ssl/sslsock.c
b/net/third_party/nss/ssl/sslsock.c
index
1823a1ca3faa0be49d628f006d3b1df8adf4fb44..7ef13389a78de373ce9ebacb8403d0e61460a5dd
100644
--- a/net/third_party/nss/ssl/sslsock.c
+++ b/net/third_party/nss/ssl/sslsock.c
@@ -2934,11 +2934,15 @@ ssl_SetDefaultsFromEnvironment(void)
ev = getenv("SSLKEYLOGFILE");
if (ev && ev[0]) {
ssl_keylog_iob = fopen(ev, "a");
- if (ftell(ssl_keylog_iob) == 0) {
- fputs("# SSL/TLS secrets log file, generated by NSS\n",
- ssl_keylog_iob);
+ if (!ssl_keylog_iob) {
+ perror("Failed open key log file");
+ } else {
+ if (ftell(ssl_keylog_iob) == 0) {
+ fputs("# SSL/TLS secrets log file, generated by NSS\n",
+ ssl_keylog_iob);
+ }
+ SSL_TRACE(("SSL: logging pre-master secrets to %s", ev));
}
- SSL_TRACE(("SSL: logging pre-master secrets to %s", ev));
}
ev = getenv("SSLBYPASS");
if (ev && ev[0]) {

[rsl...@chromium.org]

LGTM


https://chromiumcodereview.appspot.com/10636062/diff/1/net/third_party/nss/ssl/sslsock.c
File net/third_party/nss/ssl/sslsock.c (right):

https://chromiumcodereview.appspot.com/10636062/diff/1/net/third_party/nss/ssl/sslsock.c#newcode2938
net/third_party/nss/ssl/sslsock.c:2938: perror("Failed open key log
file");
nit: SSL_TRACE instead?

I don't feel strongly about this. I can understand why perror is useful
(stderr is a very visible user-surfacing of what we presume was a user
intent to log to a particular file), but I dislike library code writing
to stderr (I can only find four or five places in nss/lib that aren't
guarded by #debugs)

https://chromiumcodereview.appspot.com/10636062/

[a...@chromium.org]

https://chromiumcodereview.appspot.com/10636062/diff/1/net/third_party/nss/ssl/sslsock.c
File net/third_party/nss/ssl/sslsock.c (right):

https://chromiumcodereview.appspot.com/10636062/diff/1/net/third_party/nss/ssl/sslsock.c#newcode2938
net/third_party/nss/ssl/sslsock.c:2938: perror("Failed open key log
file");

On 2012/06/26 21:01:28, Ryan Sleevi wrote:
> nit: SSL_TRACE instead?

> I don't feel strongly about this. I can understand why perror is
useful (stderr
> is a very visible user-surfacing of what we presume was a user intent
to log to
> a particular file), but I dislike library code writing to stderr (I
can only
> find four or five places in nss/lib that aren't guarded by #debugs)

Fair enough. I fear that, most of the time, SSL_TRACE won't be defined
to do anything. But I agree that dumping to stderr isn't great so, done.

https://chromiumcodereview.appspot.com/10636062/

[commi...@chromium.org]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/a...@chromium.org/10636062/6001


https://chromiumcodereview.appspot.com/10636062/

[commi...@chromium.org]

Change committed as 144495

https://chromiumcodereview.appspot.com/10636062/