Opened 5 months ago

Closed 3 months ago

#26456 closed defect (fixed)

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, TorBrowserTeam201808R
Cc: tbb-team, mcs, brade, arthuredelstein 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 4 months ago.

Download all attachments as: .zip

Change History (15)

comment:1 Changed 5 months ago by gk

Cc: tbb-team added

comment:2 Changed 5 months ago by gk

Priority: HighVery High

comment:3 Changed 5 months 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 5 months ago by pospeselr

Keywords: ff60-esr added

comment:5 Changed 4 months ago by gk

Keywords: TorBrowserTeam201807R added; TorBrowserTeam201806R removed

Moving reviews to July.

comment:6 Changed 4 months 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 4 months ago by gk (previous) (diff)

comment:7 Changed 4 months 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.

comment:8 in reply to:  7 Changed 4 months ago by gk

Cc: mcs brade arthuredelstein added
Status: needs_revisionneeds_review

Replying to 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'.

Thanks for the explanation.

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

How did it go?

comment:9 Changed 4 months ago by gk

Keywords: TorBrowserTeam201807R added; TorBrowserTeam201807 removed

comment:10 Changed 4 months ago by gk

Keywords: TorBrowserTeam201808R added; TorBrowserTeam201807R removed

comment:11 Changed 3 months ago by pospeselr

Sorry didn't see this. Can confirm I didn't break anything.

comment:12 Changed 3 months ago by arthuredelstein

I am unsure what the comment

 // Ignore GetSSLStatus result

means. It seems like you are saving the result of GetSSLStatus, not ignoring it.

This line:

mSSLStatus = nullptr;

could be moved inside the if block above it.

Otherwise the code looks good to me.

comment:13 Changed 3 months ago by pospeselr

Update patch: https://gitweb.torproject.org/user/richard/tor-browser.git/commit/?h=bug_26456

The comment refers to the error code returned by GetSSLStatus.

comment:14 Changed 3 months ago by gk

Resolution: fixed
Status: needs_reviewclosed

Looks good. Applied to tor-browser-60.1.0esr-8.0-1 (commit 3c5e1db5bc2131872bca5ec766522787beeee869).

Note: See TracTickets for help on using tickets.