dev tools network panel shows lock on http:// alt-svc transaction with tls

RESOLVED FIXED in Firefox 39

Status

RESOLVED FIXED
4 years ago
4 months ago

People

(Reporter: mcmanus, Assigned: mcmanus)

Tracking

({sec-low})

unspecified
Firefox 40
x86_64
Linux
sec-low

Firefox Tracking Flags

(firefox39 fixed, firefox40 fixed, firefox-esr38 unaffected)

Details

(Whiteboard: [adv-main39-])

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

4 years ago
I'm not sure this is really a security sensitive bug(*), but I'll be conservative and start it that way.

Alt-Svc can allow http:// resources to be carried over TLS. Those resources do not receive lock icons in the location bar - only https:// gets that. (and for good reason - they are trivially downgradable).

However the networking panel in dev tools uses the lock icon when it sees the securityInfo structure. It should use the same logic the location bar uses (which is to enforce the scheme) for deciding whether the security info is relevant.

A patch is forthcoming. Its possible we might want to be able to communicate the security info without the lock icon, but that's a different patch.

(*) 1] its not in regular browser chrome, and 2] alt-svc is preffed off at the moment anyhow
(Assignee)

Comment 1

4 years ago
fwiw devtools is definitely the right place to convey this information (I believe the spec even mentions that possibility) - its just the lock should be reserved for https
(Assignee)

Comment 2

4 years ago
Created attachment 8591866 [details] [diff] [review]
dev tools and alt-svc
Attachment #8591866 - Flags: review?(vporof)
(Assignee)

Updated

4 years ago
Assignee: nobody → mcmanus
Status: NEW → ASSIGNED
(Assignee)

Comment 3

4 years ago
Created attachment 8592256 [details] [diff] [review]
dev tools and alt-svc
Attachment #8592256 - Flags: review?(vporof)
(Assignee)

Updated

4 years ago
Attachment #8591866 - Attachment is obsolete: true
Attachment #8591866 - Flags: review?(vporof)
(Assignee)

Comment 4

4 years ago
one of the tests actually uses a security info without a URI. I have changed the patch to allow that situation to use the legacy behavior of allowing it to be considered "secure" if there is no URI (and the other things match, of course).
Marking sec-low if feature is exposed to developers today, but I suspect since alt-svc was disabled in 37.0.1 that it is not available and might be more accurately rated as sec-other. Either way, change rating as you see fit.
Keywords: sec-low
Attachment #8592256 - Flags: review?(vporof) → review+
(Assignee)

Comment 7

3 years ago
Comment on attachment 8592256 [details] [diff] [review]
dev tools and alt-svc

with alt-svc being re-enabled on aurora 1148328 it makes sense (but is not required) to take this as well. It updates a UI indicator in dev tools in the presence of alt-svc. (sec-low).

Approval Request Comment
[Feature/regressing bug #]: alt-svc
[User impact if declined]: dev tools shows a lock for non https but in the presence of crypto. While the general alt-svc rule is no-ui for http over tls, dev-tools would be the place to make an exception so this isn't especially bad, but the choice of the same info as https is not desirable.
[Describe test coverage new/current, TreeHerder]: just regression testing.
[Risks and why]: low risk, impacting just the icon selection in network panel of dev tools.
[String/UUID change made/needed]:
Attachment #8592256 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/794849eb1d82
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
(In reply to Patrick McManus [:mcmanus] from comment #7)
> [String/UUID change made/needed]:

Two issues with this bug.

When you update a string you should bump (or change) the string ID. This didn't happen, and I'm not even sure why the string moved from "encrypted" to "secure".
https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_content_best_practices#Changing_existing_strings

You're also asking to move this change to mozilla-aurora, and it will create unnecessary noise in tools one week from merge day. Any chance to land the code without any string change?
(Assignee)

Comment 10

3 years ago
(In reply to Francesco Lodolo [:flod] (UTC+2) from comment #9)
> (In reply to Patrick McManus [:mcmanus] from comment #7)
> > [String/UUID change made/needed]:
> 
> Two issues with this bug.

thanks!

> When you update a string you should bump (or change) the string ID.

Can you provide more detail? Do you mean change the key name? Then it presumably needs to be changed in more places to match? Surely its not that hard and risk prone to update a minor piece of inaccurate content?

 This
> didn't happen, and I'm not even sure why the string moved from "encrypted"
> to "secure".

secure is https, encrypted might be a few other things.. including opportunistic encryption for example - which is encrypted but definitely not secure. The rest of the patch makes sure the dev tools lock icon is only applied to secure.

> Any chance to land the
> code without any string change?

yes. I can do that.
(Assignee)

Updated

3 years ago
Attachment #8592256 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 11

3 years ago
Created attachment 8600895 [details]
1154012-aurora

This is the same as the previous a? request except the string change has been removed.

with alt-svc being re-enabled on aurora 1148328 it makes sense (but is not required) to take this as well. It updates a UI indicator in dev tools in the presence of alt-svc. (sec-low).

Approval Request Comment
[Feature/regressing bug #]: alt-svc
[User impact if declined]: dev tools shows a lock for non https but in the presence of crypto. While the general alt-svc rule is no-ui for http over tls, dev-tools would be the place to make an exception so this isn't especially bad, but the choice of the same info as https is not desirable.

[Describe test coverage new/current, TreeHerder]: just regression testing.

[Risks and why]: low risk, impacting just the icon selection in network panel of dev tools.

[String/UUID change made/needed]: none
Attachment #8600895 - Flags: review+
Attachment #8600895 - Flags: approval-mozilla-aurora?
(In reply to Patrick McManus [:mcmanus] from comment #10)
> Can you provide more detail? Do you mean change the key name? Then it
> presumably needs to be changed in more places to match? Surely its not that
> hard and risk prone to update a minor piece of inaccurate content?

See the link above: changing the string ID (key name) is the only reliable way to make sure that localizers are aware of the change, and existing localizations are invalidated. It's annoying for both devs and localizers, but that's what we have.

> secure is https, encrypted might be a few other things.. including
> opportunistic encryption for example - which is encrypted but definitely not
> secure. 

Thanks, that explains the change.
Comment on attachment 8600895 [details]
1154012-aurora

Approved for uplift to aurora. Thanks for catching and removing the string changes.
Attachment #8600895 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Looks like Patrick landed this.
https://hg.mozilla.org/releases/mozilla-aurora/rev/096b3675b325
status-firefox39: --- → fixed
status-firefox-esr38: --- → unaffected
Whiteboard: [adv-main39-]

Updated

3 years ago
Group: core-security → core-security-release
Group: core-security-release

Updated

4 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.