1488061 - Directory indices shouldn't just echo all URL input

Status: VERIFIED FIXED

(Core :: Networking, defect, P2)

Description

[:Gijs (he/him)]

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.

Comment 1

[Valentin Gosu [:valentin] (he/him)]

Attachment: Bug 1488061 - Remove Query/Ref from the directory listing URI r=bagder!

Comment 2

[Daniel Stenberg [:bagder]]

Comment on attachment 9006071 [details]
Bug 1488061 - Remove Query/Ref from the directory listing URI r=bagder!

Daniel Stenberg [:bagder] has approved the revision.

Comment 3

[:Gijs (he/him)]

Did you mean to land this?

Comment 4

[Valentin Gosu [:valentin] (he/him)]

(In reply to :Gijs (he/him) from comment #3)
> Did you mean to land this?
Yes, thanks for reminding me.

Comment 5

[Valentin Gosu [:valentin] (he/him)]

I'm not able to land it with lando.

Comment 6

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 7

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Per the email thread, I believe this needs Beta/ESR60 approval requests?

Comment 9

[Valentin Gosu [:valentin] (he/him)]

Gijs, can you request approval for me? I'll be away for two weeks.

Comment 10

[:Gijs (he/him)]

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

Comment 11

[Pascal Chevrel:pascalc]

Comment on attachment 9006071 [details]
Bug 1488061 - Remove Query/Ref from the directory listing URI r=bagder!

Approved for 63 beta 7, thanks.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/6965bf2d3c5a

Comment 13

[Oana Botisan, Desktop QA]

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.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/050d0cfa8e3d

Comment 16

[Paul_B]

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.