HTTP .onion sites inherit previous page's certificate information
Seems to be an edge case from the #23247 (moved) 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.
Activity
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
mSSLStatusas-is in caseif (sp)is essentiallyfalse. 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.Trac:
Keywords: TorBrowserTeam201807R deleted, TorBrowserTeam201807 added
Status: needs_review to needs_revision
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.
-
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?
Trac:
Status: needs_revision to needs_review
Cc: tbb-team to tbb-team, mcs, brade, arthuredelstein -
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.
mentioned in issue #26507 (moved)