Closed Bug 1154012 Opened 9 years ago Closed 9 years ago

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

Categories

(DevTools :: Console, defect)

x86_64
Linux
defect
Not set
normal

Tracking

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

RESOLVED FIXED
Firefox 40
Tracking Status
firefox39 --- fixed
firefox40 --- fixed
firefox-esr38 --- unaffected

People

(Reporter: mcmanus, Assigned: mcmanus)

References

Details

(Keywords: sec-low, Whiteboard: [adv-main39-])

Attachments

(2 files, 1 obsolete file)

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
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
Attached patch dev tools and alt-svc (obsolete) — Splinter Review
Attachment #8591866 - Flags: review?(vporof)
Assignee: nobody → mcmanus
Status: NEW → ASSIGNED
Attachment #8592256 - Flags: review?(vporof)
Attachment #8591866 - Attachment is obsolete: true
Attachment #8591866 - Flags: review?(vporof)
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+
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
Closed: 9 years ago
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?
(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.
Attachment #8592256 - Flags: approval-mozilla-aurora?
Attached file 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+
Whiteboard: [adv-main39-]
Group: core-security → core-security-release
Group: core-security-release
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.