1017616 - (CVE-2017-5381) "export" in Certificate Viewer can cause navigation to arbitrary filesystem paths

Status: RESOLVED FIXED

(Core :: Security: PSM, defect, P1)

Description

[Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]]

Date: Thu, 29 May 2014 03:35:19 +0200
From: Jann Horn <[e-mail]>
To: [e-mail]
Subject: Clicking "export" in Certificate Viewer can cause navigation to
	arbitrary filesystem paths
-----//-----
Version tested: Firefox Nightly 31.0a1 on Debian Linux 7

VULNERABILITY DETAILS
On Linux, when a Certificate with slashes in the Common Name field is exported, this causes the file saving dialog to put the
whole CN, including slashes, into the filename field. If the user just hits the save button, this causes the file to be saved
in an attacker-chosen location on disk with an attacker-chosen filename, possibly leading to the file being interpreted as a
configuration file or similar.

REPRODUCTION CASE
Create a self-signed cert like this:
$ openssl req -x509 -nodes -days 365 -newkey rsa:2048 -keyout mysitename.key -out mysitename.crt
Generating a 2048 bit RSA private key
...............................+++
.....................................+++
writing new private key to 'mysitename.key'
-----
You are about to be asked to enter information that will be incorporated
into your certificate request.
What you are about to enter is what is called a Distinguished Name or a DN.
There are quite a few fields but you can leave some blank
For some fields there will be a default value,
If you enter '.', the field will be left blank.
-----
Country Name (2 letter code) [AU]:
State or Province Name (full name) [Some-State]:
Locality Name (eg, city) []:
Organization Name (eg, company) [Internet Widgits Pty Ltd]:
Organizational Unit Name (eg, section) []:
Common Name (e.g. server FQDN or YOUR name) []:../../../../../tmp/foobar
Email Address []:

First navigate to some normal https website and save its cert. Then configure a webserver to use the new cert you just created and navigate to that server. On the cert warning page, open the certificate viewer and click "export". You should see a "save file" dialog showing you the directory in which you saved the last cert with a suggested filename "../../../../../tmp/foobar". Click "save". Now there should be a new certificate file /tmp/foobar in your local filesystem.

Note that there is a similar issue in Chromium, Issue 378512 - you might want to coordinate the fix with them?

(Also, testing for this issue in Firefox was not my idea - I found it in Chromium, [e-mail] suggested to check Firefox for the same issue.)

Comment 1

[Daniel Veditz [:dveditz]]

Luckily there are many mitigations here:
 * users never save certs (within a negligible epsilon of "never")
 * would be hard to pick a file target that would work on multiple OS's
   (though "just windows" is often good enough)
 * the contents of the file will be the certificate, not (entirely) arbitrary content.
   Would be easy to destroy a known file but probably not usefully replace it.

Comment 2

[Jann Horn]

> users never save certs (within a negligible epsilon of "never")

Yes, I agree.

> would be hard to pick a file target that would work on multiple OS's

Well, an attacker could make an HTTP site or so that, depending on the UA string and things like that, redirects to different HTTPS servers.

> the contents of the file will be the certificate, not (entirely) arbitrary content. Would be easy to destroy a known file but probably not usefully replace it.

Well, an attacker could try to put the file in a location where a certificate is expected, e.g. in $HOME/.purple/certificates/x509/tls_peers/. That might then give him the ability to MITM TLS-protected traffic of another application, right?

Comment 3

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Looks like this is how it happens:

security/manager/pki/resources/content/pippki.js:

74 function exportToFile(parent, cert)
75 {
76   var bundle = document.getElementById("pippki_bundle");
77   if (!cert)
78     return;
79 
80   var nsIFilePicker = Components.interfaces.nsIFilePicker;
81   var fp = Components.classes["@mozilla.org/filepicker;1"].
82            createInstance(nsIFilePicker);
83   fp.init(parent, bundle.getString("SaveCertAs"),
84           nsIFilePicker.modeSave);
85   var filename = cert.commonName;
86   if (!filename.length)
87     filename = cert.windowTitle;
88   // remove all whitespace from the default filename
89   fp.defaultString = filename.replace(/\s*/g,'');

In addition to this code stripping out slashes (and maybe other non-alphanumeric characters?), it would probably be a good idea for nsIFilePicker.defaultString to disallow/prevent this sort of thing.

Comment 4

[Richard Barnes [:rbarnes]]

After looking at this for a second, ISTM that this is may *not* be something for nsIFilePicker.defaultString to address.  In the Windows flavor of nsFilePicker, there's a comment that makes it appear that there are cases where having directory separators in defaultString is desirable.

>  // Then, we need to replace illegal characters. At this stage, we cannot
>  // replace the backslash as the string might represent a file path.
>  mDefaultFilePath.ReplaceChar(FILE_ILLEGAL_CHARACTERS, '-');
>  mDefaultFilename.ReplaceChar(FILE_ILLEGAL_CHARACTERS, '-');

http://dxr.mozilla.org/mozilla-central/source/widget/windows/nsFilePicker.cpp#1130

It's only in this scenario, where we have adversarial input going into nsIFilePicker.defaultString, that we need extra checks.  

Or maybe this should be handled differently in the per-platform nsFilePicker classes.  I'm not sure if there's something special about how Windows does file pickers that requires this functionality, which wouldn't be true in MacOS or Linux.

Comment 5

[Daniel Veditz [:dveditz]]

Although this is broken it's not an immediate danger to users (they have to accept a bad cert--already a problem for them--and then take the unusual steps to export it). Does not meet the bug bounty severity bar.

Comment 6

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

(In reply to Daniel Veditz [:dveditz] from comment #5)
> Although this is broken it's not an immediate danger to users (they have to
> accept a bad cert--already a problem for them--and then take the unusual
> steps to export it). Does not meet the bug bounty severity bar.

Why does this only apply to bad certs? A trusted CA could issue a certificate with CN=/tmp/foobar, couldn't it? There are no restrictions on CN in the baseline requirements, IIRC.

Comment 8

[Matt Wobensmith [:mwobensmith][:matt:]]

Keeler, do you think this looks interesting? 

Would a fix be simple? We have Chrome's example.

Also, we are wondering if we should keep this private, since Chrome's issue was already made public.

Comment 9

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

I'm not too concerned about this bug, but it does seem like behavior we should make an attempt to prevent. I think the fix would be fairly simple (just filtering out some characters when we open the save file dialog when exporting a certificate).
This probably doesn't need to be kept private.

Comment 10

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Cykesiopka: this is near some code you're working on - any interest in addressing this as well?

Comment 11

[:Cykesiopka]

(In reply to David Keeler [:keeler] (use needinfo?) from comment #10)
> Cykesiopka: this is near some code you're working on - any interest in
> addressing this as well?

Sure.

Comment 12

[:Cykesiopka]

Attachment: bug1017616_filter-unnecessary-chars_v1.patch

Comment 13

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8799370 [details] [diff] [review]
bug1017616_filter-unnecessary-chars_v1.patch

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

Great - thanks. We really should have tests covering this functionality, but fixing this shouldn't block that. Maybe we can get away with some QA spotchecking for now.

Comment 14

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c4876903db081647838bd629627d35e839deb9a0

Comment 15

[:Cykesiopka]

(In reply to David Keeler [:keeler] (use needinfo?) from comment #13)
> Great - thanks. We really should have tests covering this functionality, but
> fixing this shouldn't block that. Maybe we can get away with some QA
> spotchecking for now.

Thanks for the review!
I took a look at adding tests to cover exporting certs, but discovered that it would probably take a while to write. I'm trying to add tests for all of our UI under security/manager/pki/ anyways, so I'll probably get to it eventually.

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c4876903db08

Comment 17

[:Cykesiopka]

[Tracking Requested - why for this release]:
This is a long-standing security vulnerability.
Since the fix is relatively simple and contained, it would be nice to uplift it everywhere.

Comment 18

[Gerry Chang [:gchang]]

Hi :dveditz,
Do you think this is worth uplifting to 50/51?

Comment 19

[Daniel Veditz [:dveditz]]

Release Management won't want to uplift to beta this late in the cycle, but it's probably worth requesting aurora (51) approval.

Comment 20

[:Cykesiopka]

Comment on attachment 8799370 [details] [diff] [review]
bug1017616_filter-unnecessary-chars_v1.patch

Approval Request Comment
[Feature/regressing bug #]:
None. Long-standing vulnerability.

[User impact if declined]:
An attacker or a compromised CA could cause a certificate to be saved in a location unexpected by the user, when the user tries to export said certificate.
See comment 0 and comment 2 for the impact this can have.

[Describe test coverage new/current, TreeHerder]:
None. Manually tested by following similar steps to those described in comment 0.

[Risks and why]:
Low. The change is simple and contained, and touches functionality that is unused by most.

[String/UUID change made/needed]:
None.

Comment 21

[Gerry Chang [:gchang]]

Comment on attachment 8799370 [details] [diff] [review]
bug1017616_filter-unnecessary-chars_v1.patch

Fix a sec-moderate issue. Take it in 51 aurora.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3f38f335bb38

Comment 23

[:Cykesiopka]

Attachment: bug1017616_filter-unnecessary-chars_ESR45_v1.patch

[Approval Request Comment]
See comment 20.

Comment 24

[Liz Henry (:lizzard) (relman/hg->git project)]

We don't generally uplift fixes for sec-moderate issues to ESR. 
Dan, do you have an opinion on this? Can it wait for 52esr?

Comment 25

[Daniel Veditz [:dveditz]]

We don't need this fix on ESR-45