Closed Bug 1488061 Opened 2 years ago Closed 2 years ago

Directory indices shouldn't just echo all URL input

Categories

(Core :: Networking, defect, P2)

defect

Tracking

()

VERIFIED FIXED
mozilla64
Tracking Status
firefox-esr60 63+ verified
firefox62 --- wontfix
firefox63 + verified
firefox64 + verified

People

(Reporter: Gijs, Assigned: valentin)

References

Details

(Keywords: csectype-other, sec-moderate, Whiteboard: [necko-triaged][post-critsmash-triage][adv-main63+][adv-esr60.3+])

Attachments

(1 file)

Follow-up from bug 1460538:

STR:

1. open:

   chrome://browser/content/places/?%27*{}*{color:red}

as a toplevel URL


AR:

we show a directory index of the places folder, but we echo the query string into the resulting HTML as-is.

ER:

we shouldn't echo the query string or hash.


We may also want to consider enforcing other restrictions on chrome/resource URLs (e.g. has to match #^[.a-z0-9/_-]*$#, so only latin alphanumeric, forward slashes as path separators, '.', '_' and '-', nothing else). We probably can't reliably enforce any restrictions on file:/// URIs, so sadly probably can't enforce such restrictions on all directory indices.
Assignee: nobody → valentin.gosu
Whiteboard: [necko-triaged]
Comment on attachment 9006071 [details]
Bug 1488061 - Remove Query/Ref from the directory listing URI r=bagder!

Daniel Stenberg [:bagder] has approved the revision.
Attachment #9006071 - Flags: review+
Did you mean to land this?
Flags: needinfo?(valentin.gosu)
(In reply to :Gijs (he/him) from comment #3)
> Did you mean to land this?
Yes, thanks for reminding me.
Flags: needinfo?(valentin.gosu)
Priority: -- → P2
I'm not able to land it with lando.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e414fcdabb72
Group: network-core-security → core-security-release
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Per the email thread, I believe this needs Beta/ESR60 approval requests?
Flags: needinfo?(valentin.gosu)
Gijs, can you request approval for me? I'll be away for two weeks.
Flags: needinfo?(valentin.gosu) → needinfo?(gijskruitbosch+bugs)
Comment on attachment 9006071 [details]
Bug 1488061 - Remove Query/Ref from the directory listing URI r=bagder!

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
Security issue that we've landed fixes for on trunk.
User impact if declined: potential for CSP bypass
Fix Landed on Version: 63 (bug 1460538) / 64 (bug 1488061)
Risk to taking this patch (and alternatives if risky): very low - this reduces what parts of the URI we display in directory listings.
String or UUID changes made by this patch: n/a

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Approval Request Comment
[Feature/Bug causing the regression]: security issue
[User impact if declined]: potential for security risk
[Is this code covered by automated tests?]: There are some tests that would have caught catastrophic breakage of chrome/resource directory lists, yes
[Has the fix been verified in Nightly?]: not yet
[Needs manual test from QE? If yes, steps to reproduce]: see comment #0
[List of other uplifts needed for the feature/fix]: n/a
[Is the change risky?]: no
[Why is the change risky/not risky?]: see above.
[String changes made/needed]: n/a
Flags: needinfo?(gijskruitbosch+bugs)
Attachment #9006071 - Flags: approval-mozilla-esr60?
Attachment #9006071 - Flags: approval-mozilla-beta?
Comment on attachment 9006071 [details]
Bug 1488061 - Remove Query/Ref from the directory listing URI r=bagder!

Approved for 63 beta 7, thanks.
Attachment #9006071 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
Whiteboard: [necko-triaged] → [necko-triaged][post-critsmash-triage]
I managed to reproduce the issue using an older version of Nightly (2018-09-02) on Ubuntu  16.04 x64. The tittle of the index was: Index of jar:file:///tmp/tmpGXTl8x/firefox/browser/omni.ja!/chrome/browser/content/browser/places/?'*{}*{color:red}.

I retested everything using latest Nightly 64.0a1 and beta 63.0b7 on Windows 10 x64, macOS 10.13 and Ubuntu 16.04 x64 and aftet the fix the tittle is: Index of jar:file:///home/oana.botisan/Desktop/latest Nightly/firefox/browser/omni.ja!/chrome/browser/content/browser/places/. The tittle contained only latin alphanumeric, forward slashes as path separators. I think this bug is fixed.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Comment on attachment 9006071 [details]
Bug 1488061 - Remove Query/Ref from the directory listing URI r=bagder!

Fixes a networking sec issue. Approved for ESR 60.3.
Attachment #9006071 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
QA Contact: jduell.mcbugs
I managed to reproduce the issue using version 60.2.0ESR (20180903060751). The tittle of the index was: Index of jar:file:///home/paul/Desktop/firefox/browser/omni.ja!/chrome/browser/content/browser/places/?'*{}*{color:red}.

I retested using 60.2.3ESR (2018100421210422) on Windows 10 x64, macOS 10.13.6 and Ubuntu 16.04 x64 and the tittle was: Index of jar:file:///home/paul/Desktop/firefox/firefox/browser/omni.ja!/chrome/browser/content/browser/places/. The issue is fixed -> verified on 60.2.3 ESR.
Flags: qe-verify+
QA Contact: jduell.mcbugs
Whiteboard: [necko-triaged][post-critsmash-triage] → [necko-triaged][post-critsmash-triage][adv-main63+][adv-esr60.3+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.