941354 - Several issues with view-source:https://<site with bad cert>

Status: VERIFIED FIXED

(Firefox :: Security, defect, P2)

Description

[Martin Thomson [:mt:]]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:26.0) Gecko/20100101 Firefox/26.0 (Beta/Release)
Build ID: 20131114085019

Steps to reproduce:

Load a page with a self-signed certificate, but prefix with view-source:, e.g.:
view-source:https://www.roach.at/
Attempt to add an exception.


Actual results:

The exception dialog appears, but the certificate never loads.  The only non-greyed out button is 'Cancel'.


Expected results:

Just as for URLs without the view-source: prefix.

Comment 1

[Paul Silaghi, QA [:pauly]]

Confirmed 28.0a1 (2013-12-03), Win 7 x64.
The "Confirm Security Exception" button is grayed out for view-source:https://www.roach.at/

Comment 2

[:Cykesiopka]

I tested this on an Aurora/FDE 41 Win7 build and an m-i Linux64 build, and it looks
like the certificate now loads, but pressing the "Confirm Security Exception" does nothing.

On the m-i build, this error appears:
> JavaScript error: chrome://pippki/content/exceptionDialog.js, line 149: NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIURI.port]

Comment 4

[u564464]

Reproducible on 51.0.1 on 45.7.0 ESR

Go to view-source:https://cnn.com/ , Add Exception.
Clicking Confirm Security Exception does nothing. Location box contains "view-source:https://cnn.com/", if you change it to "https://cnn.com/" the Confirm Security Exception button gets greyed out.

Comment 5

[Paul Silaghi, QA [:pauly]]

Confirmed on 54.0a1 (2017-02-13) (32-bit), Win 7.

Comment 7

[Johann Hofmann [:johannh]]

Here's my proposition on how to solve this. Since RememberValidityOverride doesn't seem to play well with view-source: URLs (I haven't really dug deep into why there's an error), the easiest thing to do here would be to remove any leading view-source: schemes when pre-filling the full URL into the dialog input, e.g. shortening view-source:https://cnn.com/ to just https://cnn.com/. I think this better represents what the user actually wants to do and it clearly conveys to the user that they're adding an exception for the site.

David, does that sound good to you? If so, I'll make this a mentored bug so that we can finally get it resolved.

Comment 8

[Dana Keeler (she/her) (use needinfo) [:keeler]]

That seems reasonable.

Comment 9

[Johann Hofmann [:johannh]]

Alright, thanks.

Whoever wants to fix this will probably need to change the behavior here: http://searchfox.org/mozilla-central/rev/ef0b6a528e0e7b354ddf8cdce08392be8b8ca679/security/manager/pki/resources/content/exceptionDialog.js#65

where we assign the locationTextBox value. It should shorten the url as described in comment 7.

Comment 10

[anejaalisha]

Could you please assign this bug to me?

Comment 11

[Johann Hofmann [:johannh]]

Done! Let me know if you have any questions...

Have fun!

Comment 12

[anejaalisha]

Attachment: Bug941354.patch

Comment 13

[Johann Hofmann [:johannh]]

Comment on attachment 8851308 [details] [diff] [review]
Bug941354.patch

Review of attachment 8851308 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, just one minor thing:

::: security/manager/pki/resources/content/exceptionDialog.js
@@ +61,5 @@
>    var args = window.arguments;
>    if (args && args[0]) {
>      if (args[0].location) {
>        // We were pre-seeded with a location.
> +      if (args[0].location.startsWith("view-source:")) {

Please put args[0].location in a local variable instead of repeating it.

Comment 14

[anejaalisha]

Attachment: Bug941354.patch

Comment 15

[Johann Hofmann [:johannh]]

Comment on attachment 8851532 [details] [diff] [review]
Bug941354.patch

Review of attachment 8851532 [details] [diff] [review]:
-----------------------------------------------------------------

Please use let instead of var. And you can just call the variable location instead of url_location.

Also, please don't just replace two occurrences of args[0].location, but rather all in that block.

Thanks!

Comment 16

[anejaalisha]

Attachment: Bug941354.patch

I was bit confused on where to scope the variable. But I did according to my understanding. 
Kindly provide feedback on it.
Thanks!

Comment 17

[Johann Hofmann [:johannh]]

Comment on attachment 8853590 [details] [diff] [review]
Bug941354.patch

Review of attachment 8853590 [details] [diff] [review]:
-----------------------------------------------------------------

The scope looks good but you can simplify the code by assigning

let location = args[0].location;

instead of using temp_args everywhere. :)

Thanks!

Comment 18

[anejaalisha]

Attachment: Bug941354.patch

Done:)

Comment 19

[Johann Hofmann [:johannh]]

Comment on attachment 8854685 [details] [diff] [review]
Bug941354.patch

Review of attachment 8854685 [details] [diff] [review]:
-----------------------------------------------------------------

The variable handling looks great now! There's just one small thing left that we need to change:

::: security/manager/pki/resources/content/exceptionDialog.js
@@ +63,5 @@
> +    // We were pre-seeded with a location.
> +    let location = args[0].location;
> +    if (location) {
> +      if (location.startsWith("view-source:")) {
> +        location = location.split("view-source:")[1];

Please use .replace("view-source:", "") instead of split("view-source:")[1]. Here's why:

In a very contrived example, you could set up a web server with an invalid certificate on

https://google.comview-source:8000/

and then link some important content to view-source:https://google.comview-source:8000/

When the user wants to open that file and adds an exception to view it, your code does the following:

$ "view-source:https://google.comview-source:8000/".split("view-source:")
> Array [ "", "https://google.com", "8000/" ]

Which results in "https://google.com" being selected for adding a certificate exception. I know I often forget to double-check the domain in the dialog when I add a security exception.

.replace will look like this instead:

$ "view-source:https://google.comview-source:8000/".replace("view-source:", "")  
> "https://google.comview-source:8000/"

because it only replaces the first occurrence when passed a string to replace.

I hope that makes sense :)

Thanks!

Comment 20

[Johann Hofmann [:johannh]]

Ok, trying this out I noticed that we don't allow linking to view-source: URLs, which makes this a bit less viable as an attack. I'd still not risk it. :)

Comment 21

[anejaalisha]

Attachment: Bug941354.patch

Comment 22

[Johann Hofmann [:johannh]]

Comment on attachment 8854863 [details] [diff] [review]
Bug941354.patch

Review of attachment 8854863 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me, but I'm not sure if I can actually review in this component. Requesting another review from :keeler to be sure. (Sorry for the delay).

Comment 23

[Johann Hofmann [:johannh]]

In the meantime I will trigger a try run so that we know all the tests are still passing (https://wiki.mozilla.org/ReleaseEngineering/TryServer).

Comment 24

[Johann Hofmann [:johannh]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=be51b2b0f8c5ed55cc1c9b571c8906f5c7f87af2

Comment 25

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8854863 [details] [diff] [review]
Bug941354.patch

Review of attachment 8854863 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you for your work on this, but I don't think this is quite the right location for this change. See comment.
Also, it would be great to have a test for this. I think you can easily add one by adding a task similar to this one: https://dxr.mozilla.org/mozilla-central/rev/45692c884fdd5136a64fb2f8a61a0c8183b69331/browser/base/content/test/general/browser_addCertException.js#14 except the url would be "view-source:https://expired.example.com".

::: security/manager/pki/resources/content/exceptionDialog.js
@@ +63,5 @@
> +    // We were pre-seeded with a location.
> +    let location = args[0].location;
> +    if (location) {
> +      if (location.startsWith("view-source:")) {
> +        location = location.replace("view-source:", "");

I actually don't think this is the right place to do this. Either the code that opens this dialog needs to do the check and strip out the "view-source:" (and there are various places that happens, so that's not the easiest route) or this should be part of getURI() (as in, in creating the URI, the function should ignore any leading "view-source:" - in that case, it wouldn't change the value of the text field).

Comment 26

[Johann Hofmann [:johannh]]

anejaalisha, I'm unassigning you due to inactivity. Feel free to pick this up again if you like.

(In reply to David Keeler 
[:keeler] (use needinfo?) from comment #25)
> Comment on attachment 8854863 [details] [diff] [review]
> Bug941354.patch
> ::: security/manager/pki/resources/content/exceptionDialog.js
> @@ +63,5 @@
> > +    // We were pre-seeded with a location.
> > +    let location = args[0].location;
> > +    if (location) {
> > +      if (location.startsWith("view-source:")) {
> > +        location = location.replace("view-source:", "");
> 
> I actually don't think this is the right place to do this. Either the code
> that opens this dialog needs to do the check and strip out the
> "view-source:" (and there are various places that happens, so that's not the
> easiest route) or this should be part of getURI() (as in, in creating the
> URI, the function should ignore any leading "view-source:" - in that case,
> it wouldn't change the value of the text field).

You're just listing alternative locations without explaining why this is the wrong location for that change. Can you elaborate?

Comment 27

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Eh, I think my first suggestion was just wrong. This dialog should handle invalid input wherever it gets it from, which I think means we'll have to put that code in getURI.

Comment 28

[Jawad Ahmed [:jawad]]

Hi, adding location = location.replace(/^view-source:/i,""); in onCertError method https://searchfox.org/mozilla-central/source/browser/base/content/browser.js#2971 seems to fix the issue. The certificate loads and exception added. Should i work on it?

Comment 29

[Johann Hofmann [:johannh]]

Hi Jawad, you should feel free to pick this up, but note that Dana suggested earlier that we should do this transformation in https://searchfox.org/mozilla-central/rev/3e0f1d95fcf8832413457e3bec802113bdd1f8e8/security/manager/pki/resources/content/exceptionDialog.js#138

I hope you can work with that, let me know if you have any questions.

Comment 30

[Jawad Ahmed [:jawad]]

Attachment: Bug 941354 - Fixed 'Add Exception' button for view-source: r=johannh

Comment 31

[Jawad Ahmed [:jawad]]

Hi, in latest nightly the exception dialog is not shown any more instead nightly uses a new page where exceptions are added just by clicking the "Add Exception" button. But the problem remains same that viewing certificate and adding exception doesn't work on "view-source:" pages.

Fixing dialog:

Step: 1
To first fix the exception dialog we first need to turn the browser.security.newcerterrorpage.enabled in about:config to false in order to get the dialog shown. But the dialog still doesn't show while viewing source, clicking on the button "Add Exception" does nothing.

Step: 2
I removed the "view-source:" from address bar, reloaded the page and clicked on the "Add Exception" & the dialog appeared. I clicked the get certificate and it works but then i added "view-source:" at beginning of URL and it never loads certificate.

Step:3
then i added the following lines to https://searchfox.org/mozilla-central/rev/3e0f1d95fcf8832413457e3bec802113bdd1f8e8/security/manager/pki/resources/content/exceptionDialog.js#143

let locationTextBoxValue = locationTextBox.value.replace(/^view-source:/i,"");
let uri = Services.uriFixup.createFixupURI(locationTextBoxValue, 0);

I repeated the step 2 and now it works well there, but this just fixed the problem of exception dialog and the "Add Exception" button on the "view-source:" pages still doesn't work and certificate doesn't load.

Step: 4
To get the exception dialog on the "view-source" pages following are problems we were facing.

In https://searchfox.org/mozilla-central/source/browser/base/content/browser.js#3036 there is onCertError method which also has the "location" parameter.

This method is called by receiveMessage in https://searchfox.org/mozilla-central/source/browser/base/content/browser.js#2988 which then passes location to onCertError which it gets from onCertError in https://searchfox.org/mozilla-central/source/browser/actors/NetErrorChild.jsm#782.

This is from where the raw location is sent, so by replacing

"location: win.document.location.href" with
"location: win.document.location.href.replace(/^view-source:/i,"")" in https://searchfox.org/mozilla-central/source/browser/actors/NetErrorChild.jsm#784

We get the dialog on the "view-source:" pages. The certificate loads and exception addition works.

---

This problem can also be solved by only following step 1 & 4. So if we enable the browser.security.newcerterrorpage.enabled in about:config to check with the nightly's new certificate error page, we find that the problem is also solved there.

Comment 32

[Dana Keeler (she/her) (use needinfo) [:keeler]]

If you're asking me to review your patch, I would defer to Johann.

Comment 34

[Gingerbread Man]

:johannh a duplicate was just filed for this bug report that hasn't seen any activity in over a month. Please see the patch and the latest comments (comment 30 onward).

Comment 35

[Johann Hofmann [:johannh]]

I already reviewed the patch.

Hey Jawad, any updates on this? :)

Thanks!

Comment 36

[Johann Hofmann [:johannh]]

Unassigning due to inactivity, let me know if you want to pick this up again :)

Comment 37

[aarushivij]

Hey Johannh ,I was trying to reproduce the issue, by using the url mentioned in the description.
But the url just shows a blank page!
Also can I take up this issue, if it still persists?
Thanks :)
Aarushi

Comment 38

[Johann Hofmann [:johannh]]

Yeah I'm seeing the same thing, it's pretty weird. When I go to view-source:https://expired.badssl.com on a fresh profile it will show a blank page, but it suddenly starts working fine if I visit https://expired.badssl.com directly first. Then I can also add an exception on the view-source page, but it will forward to https://expired.badssl.com. Removing doesn't work either.

Needs some investigation.

Comment 40

[:Gijs (he/him)]

Johann, can you resummarize this? :-)

Comment 41

[Johann Hofmann [:johannh]]

Known issues now that we can look at the page again:

• The "Accept the Risk and Continue" button doesn't work
• The domain isn't correctly displayed in many cases. e.g. Nightly detected an issue and did not continue to . In the Advanced section it says The certificate for expired on 4/13/2015.
• When adding an exception (via adding an exception for the non "view-source" url), clicking on "Remove Exception" in the identity popup when you're on the view-source page again will do nothing.

These seem very related (probably some mistaken URL parsing) so we can probably handle them all in this bug.

Comment 42

[Johann Hofmann [:johannh]]

Aarushi, are you still interested in this? :)

Comment 43

[aarushivij]

Yes sure :)

Comment 44

[aarushivij]

Yes sure :)

Comment 45

[aarushivij]

Attachment: Bug 941354 - Several issues with view-source:https://<site with bad cert> r=johannh

Comment 46

[Johann Hofmann [:johannh]]

We should probably find someone to finish this up...

Comment 47

[Johann Hofmann [:johannh]]

Attachment: Bug 941354 - Use innerMostURI on about:{neterror,certerror}. r=prathiksha,baku

This is to prevent issues with parsing the correct hostname for displaying and adding
exceptions for urls like view-source:.

Comment 48

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2edd2bd119ff
Use innerMostURI on about:{neterror,certerror}. r=prathiksha,baku

Comment 49

[Atila Butkovits]

Backed out for browser-chrome and devtools related failures.

Backout link: https://hg.mozilla.org/integration/autoland/rev/108176ff87a46c7228b1b7e4b4cfa0ba037366a3

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&revision=2edd2bd119ff974c8f07e30beeb02fd5d9fb7996&searchStr=bc&selectedTaskRun=Z6JRH3SETb29K6pPrrK44w.0

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=319795100&repo=autoland&lineNumber=3191
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=319794799&repo=autoland&lineNumber=3298
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=319796316&repo=autoland&lineNumber=3990

Comment 50

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/739ba4d3ca93
Use innerMostURI on about:{neterror,certerror}. r=prathiksha,baku

Comment 51

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/739ba4d3ca93

Comment 52

[Bogdan Maris, Desktop Test Engineering]

Reproduced the initial issue using old Nightly from 2020-10-20. Verified that the issues mentioned in comment 41 are fixed using Firefox 84.0b4 across platforms (Windows 7 64bit, Windows 10 64bit, macOS 11 and Ubuntu 18.04).