Opened 4 weeks ago

Last modified 32 hours ago

#26456 needs_revision defect

HTTP .onion sites inherit previous page's certificate information

Reported by: pospeselr Owned by: pospeselr
Priority: Very High Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: ff60-esr, TorBrowserTeam201807
Cc: tbb-team Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Seems to be an edge case from the #23247 patch. To reproduce:

  • Navigate to a vanilla or .onion HTTPS site
  • From same tab, navigate to a .onion HTTP site

The HTTP .onion will have the onion+lock icon, and the Page Info pane will be full of the previous HTTPS site's certificate information.

Child Tickets

TicketStatusOwnerSummaryComponent
#26507closedtbb-teamWhen I go from duckduckgo to Whoxix the certificate page shows the exact same certificate for both pagesApplications/Tor Browser

Attachments (1)

0001-Bug-26456-HTTP-.onion-sites-inherit-previous-page-s-.patch (3.3 KB) - added by pospeselr 32 hours ago.

Download all attachments as: .zip

Change History (8)

comment:1 Changed 4 weeks ago by gk

Cc: tbb-team added

comment:2 Changed 3 weeks ago by gk

Priority: HighVery High

comment:3 Changed 3 weeks ago by pospeselr

Keywords: TorBrowserTeam201806R added; TorBrowserTeam201806 removed
Status: assignedneeds_review

Found the bit of logic that was holding on to the previous page's SSL info when transitioning to a non-SSL 'secure' page (ie an onion page) and fixed it.

comment:4 Changed 2 weeks ago by pospeselr

Keywords: ff60-esr added

comment:5 Changed 2 weeks ago by gk

Keywords: TorBrowserTeam201807R added; TorBrowserTeam201806R removed

Moving reviews to July.

comment:6 Changed 2 days ago by gk

Keywords: TorBrowserTeam201807 added; TorBrowserTeam201807R removed
Status: needs_reviewneeds_revision

One nit and one concern/question. The nit:

if(mSSLStatus != nullptr) {

please add a whitespace after if.

So, the current code seems to keep the mSSLStatus as-is in case if (sp) is essentially false. I wonder if that is intentional and a use-case we should keep in mind (your patch is getting rid of that possibility). Is there a way we can reach that scenario? It seems to me the answer is "Yes", just by looking at the way the code is written. However, I am not sure which transition from load A to load B would match this in reality. It worries me that we are missing something here, so it might be worth double-checking.

Last edited 2 days ago by gk (previous) (diff)

comment:7 Changed 32 hours ago by pospeselr

So (in the original code)the updateStatus flag does 2 things:

  • first, it's used to determine whether mSSLStatus needs to be updated with the new cert info if the incoming info (nsISupports) is an nsISSLStatus
  • second, it's passed on down to UpdateSecurityState where it is OR'd with other flags to determine whether a notification needs to go out that security info has changed.

If the 'STATE_IS_SECURE' flag is set, than the mSSLStatus is cleared out later on in UpdateSecurityState. The changes in the patch force the mSSLStatus to get null'd out early since the later check will fail because onion domains get the 'STATE_IS_SECURE' flag, even without SSL info.

The patch makes it so HTTP onion pages clear out the mSSLStatus based on whether 'info' is an nsISSLStatusProvider. For vanilla HTTP pages, mSSLStatus is now cleared out twice: once based on 'info' (as with HTTP onion pages) and once again when the security flags change to 'lis_no_security'.

That all said, I'll run this (and the previous patch) through the firefox try server and verify we haven't broken anything.

Note: See TracTickets for help on using tickets.