Bug 1017616 (CVE-2017-5381)

"export" in Certificate Viewer can cause navigation to arbitrary filesystem paths

RESOLVED FIXED in Firefox 51

Status

()

P1
normal
RESOLVED FIXED
5 years ago
a year ago

People

(Reporter: curtisk, Assigned: Cykesiopka)

Tracking

({csectype-priv-escalation, sec-moderate})

31 Branch
mozilla52
All
Linux
csectype-priv-escalation, sec-moderate
Points:
---
Bug Flags:
sec-bounty -
qe-verify -

Firefox Tracking Flags

(firefox49 wontfix, firefox-esr45- wontfix, firefox50- wontfix, firefox51+ fixed, firefox52 fixed)

Details

(Whiteboard: [psm-assigned][post-critsmash-triage][adv-main51+])

Attachments

(2 attachments)

Date: Thu, 29 May 2014 03:35:19 +0200
From: Jann Horn <jann@thejh.net>
To: security@mozilla.org
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, wfh@chromium.org suggested to check Firefox for the same issue.)
Flags: sec-bounty?

Updated

5 years ago
Keywords: csectype-priv-escalation, sec-moderate
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.
Status: UNCONFIRMED → NEW
Component: Security → Security: PSM
Ever confirmed: true

Comment 2

5 years ago
> 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?
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.
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.
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.
Flags: sec-bounty? → sec-bounty-
(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.

Updated

3 years ago
Group: core-security → crypto-core-security
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.
Flags: needinfo?(dkeeler)
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.
Flags: needinfo?(dkeeler)
Whiteboard: [psm-backlog]
Cykesiopka: this is near some code you're working on - any interest in addressing this as well?
Flags: needinfo?(cykesiopka.bmo)
(Assignee)

Comment 11

2 years ago
(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.
Assignee: nobody → cykesiopka.bmo
Status: NEW → ASSIGNED
Flags: needinfo?(cykesiopka.bmo)
Priority: -- → P1
Whiteboard: [psm-backlog] → [psm-assigned]
(Assignee)

Comment 12

2 years ago
Created attachment 8799370 [details] [diff] [review]
bug1017616_filter-unnecessary-chars_v1.patch
Attachment #8799370 - Flags: review?(dkeeler)
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.
Attachment #8799370 - Flags: review?(dkeeler) → review+
(Assignee)

Comment 15

2 years ago
(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.
https://hg.mozilla.org/mozilla-central/rev/c4876903db08
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
(Assignee)

Comment 17

2 years ago
[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.
status-firefox49: --- → affected
status-firefox50: --- → affected
status-firefox51: --- → affected
status-firefox-esr45: --- → affected
tracking-firefox50: --- → ?
tracking-firefox51: --- → ?
tracking-firefox-esr45: --- → ?
Hi :dveditz,
Do you think this is worth uplifting to 50/51?
tracking-firefox51: ? → +
Flags: needinfo?(dveditz)
Release Management won't want to uplift to beta this late in the cycle, but it's probably worth requesting aurora (51) approval.
status-firefox49: affected → wontfix
Flags: needinfo?(dveditz)
(Assignee)

Comment 20

2 years ago
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.
Attachment #8799370 - Flags: approval-mozilla-aurora?
Comment on attachment 8799370 [details] [diff] [review]
bug1017616_filter-unnecessary-chars_v1.patch

Fix a sec-moderate issue. Take it in 51 aurora.
Attachment #8799370 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/3f38f335bb38
status-firefox50: affected → wontfix
status-firefox51: affected → fixed
Group: crypto-core-security → core-security-release
(Assignee)

Comment 23

2 years ago
Created attachment 8804161 [details] [diff] [review]
bug1017616_filter-unnecessary-chars_ESR45_v1.patch

[Approval Request Comment]
See comment 20.
Attachment #8804161 - Flags: review+
Attachment #8804161 - Flags: approval-mozilla-esr45?
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?
Flags: needinfo?(dveditz)
status-firefox-esr45: affected → wontfix
Attachment #8804161 - Flags: approval-mozilla-esr45? → approval-mozilla-esr45-
We don't need this fix on ESR-45
Flags: needinfo?(dveditz)
tracking-firefox50: ? → -
tracking-firefox-esr45: ? → -
Flags: qe-verify-
Whiteboard: [psm-assigned] → [psm-assigned][post-critsmash-triage]
Whiteboard: [psm-assigned][post-critsmash-triage] → [psm-assigned][post-critsmash-triage][adv-main51+]
Alias: CVE-2017-5381
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.