1597029 - Media loaded via TLS displayed as unencrypted

Status: VERIFIED FIXED

(Firefox :: Security, defect, P1)

Description

[sven-mozilla]

Attachment: screenshot

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:71.0) Gecko/20100101 Firefox/71.0

Steps to reproduce:

1. Open a direct link to an image via an https-URL (e. g. https://www.faceandfeet-lounge.de/images/banner.jpg)
2. Open page information (ctrl/cmd + I)
3. Go to tab security

Actual results:

1. Page is shown as not encrypted in spite is was delivered via HTTPS by the server.
2. Button to show certificate is not displayed

(see attached screenshot)

Expected results:

Show how the truth about the HTTPS connection and let users see the certificate chain.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

Bugbug thinks this bug should belong to this component, but please revert this change in case of error.

Comment 2

[Pascal Chevrel:pascalc (PTO until Sept 2)]

Wennie, could you help us triage and find an owner for this regression in 71 please? Thanks

Comment 3

[Wennie]

Hi J.C, please take a look at this bug. Thanks!

Comment 4

[Pascal Chevrel:pascalc (PTO until Sept 2)]

We have shipped our last beta for 71, so this is wontfix for 71.

Comment 5

[Johann Hofmann [:johannh]]

I can take a look.

Comment 6

[J.C. Jones [:jcj] (he/they)]

Johann - the path of my suspicion for this one is the changes from Bug 1468222 to nsITransportSecurityInfo.

Comment 7

[Johann Hofmann [:johannh]]

Huh, so, the regression range is wrong?

Comment 8

[J.C. Jones [:jcj] (he/they)]

Uhhh, no, probably not. Nevermind.

Comment 9

[Timea Cernea [:tbabos][inactive]]

Reproduced on latest Nightly 73.0a1 (2019-12-03) (64-bit) on Windows 10. Also did the regression range and indeed Bug 1576960 regressed this.

Comment 10

[Liz Henry (:lizzard) (relman/hg->git project)]

We could still potentially get a fix into 72, Johann is it something you may be able to work on in the next week or should I wontfix for 72?

Comment 12

[Johann Hofmann [:johannh]]

So the issue seems to be that Document::GetSecurityInfo will sometimes return null even though the corresponding channel has a valid securityInfo object. I suspect the reason is that we're only updating mSecurityInfo for HTML documents, not media documents (so videos are affected by this bug, too)

I suspect that we can just move that code to Document::StartDocumentLoad...

Comment 13

[Johann Hofmann [:johannh]]

Attachment: Bug 1597029 - Set securityInfo for all document types. r=baku

In StartDocumentLoad we copy the securityInfo reference from the load channel into a document
member variable. This used to happen only for HTML documents, but other documents (e.g. media)
can be loaded via secure channels, too and thus should have securityInfos. We're using the
securityInfo object to display information in the UI, which would previously fail for images and videos.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0c6fe9a1e6c7
Set securityInfo for all document types. r=baku

Comment 15

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/0c6fe9a1e6c7

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for Beta approval if you think it's worth doing.

Comment 17

[Johann Hofmann [:johannh]]

Comment on attachment 9119736 [details]
Bug 1597029 - Set securityInfo for all document types. r=baku

Beta/Release Uplift Approval Request

• User impact if declined: Missing security information on page info for some document types (images, videos)
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: See description, but please also try opening a video in the browser
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Simple, tested patch that moves setting a member attribute on the document to a parent method
• String changes made/needed: None

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9119736 [details]
Bug 1597029 - Set securityInfo for all document types. r=baku

Fixes missing security information for some pages. Approved for 73.0b5.

Comment 19

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/releases/mozilla-beta/rev/4fd54509b2f0

Comment 20

[Cristian Baica [:cbaica], Release Desktop QA]

Managed to reproduce the issue using Fx73.0b4.
The issue is verified fixed using the latest Fx74.0a1 and Fx73.0b5 (treeherder build) on Windows 10, Ubuntu 18 and macOS 10.13. The connection now appears as encrypted and the View Certificate button is displayed.