1077954 - the "Add Security Exception" dialog is not robust against unexpected input

Status: REOPENED

(Core :: Security: PSM, defect, P3)

Description

[YF (Yang)]

Steps to reproduce:
Open an Add Security Exception window, get certificate that filled the status2 and even status3 with a few error.

No change the location, get certificate that that filled the status1 with good status only.

Actual results:
The additional status fields not be cleared, the show like "Valid Certificate" and "Unknown Identity" at the same time.


Expected results:
Proper cleanup previous results.

See the http://hg.mozilla.org/mozilla-central/file/b85c260821ab/security/manager/pki/resources/content/exceptionDialog.js#l288.

Comment 1

[:Cykesiopka]

Attachment: bug1077954_example.png

FWIW, this is an example of how the bug can manifest. In particular, the only header and text block that should be shown is:

No Information Available
Unable to obtain identification status for this site.

(Ignore the exact wording - this is from a test build for Bug 1057035)

Comment 2

[Tummala Dhanvi [:c0mrad3][UTC+5.30]]

I would like to work on this bug 

can you please assign this one to me and say where to start and who is gona be the mentor for this bug ?

Comment 3

[YF (Yang)]

You're welcome.

I'm not sure it should have a mentor, but I'll try to help you, and you can refer to https://developer.mozilla.org/en-US/docs/Introduction.

Comment 4

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

Tummala, are you still interested in working on this?

Comment 5

[Tummala Dhanvi [:c0mrad3][UTC+5.30]]

(In reply to David Keeler [:keeler] (use needinfo?) from comment #4)
> Tummala, are you still interested in working on this?

David I am still interested in working on this bug, it's just that I need some pointers on where to look at

Comment 6

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

Actually, it looks like this has already been fixed (I can't reproduce the original issue). Sorry for the false-start, Tummala. Feel free to reach out to me if there's another bug you're interested in working on.

Comment 7

[YF (Yang)]

I guess this problem persists, just need a good testcase.

Comment 8

[Tummala Dhanvi [:c0mrad3][UTC+5.30]]

Attachment: bug1077954-new-image.png

Is this is correct re-production of the bug? 

I didn't fully understand the "get certificate that filled the status2 and even status3 with a few error." part

Comment 9

[YF (Yang)]

1. Add "104.154.89.105 example.org" (is expired.badssl.com) to your hosts file.
2. Go to Options > Advanced > Encryption > View Certificates > Servers, click Add Exception button.
3. Get the status for example.org, you will see the status2 field.

It will not be cleared, such as when you reload the state, the state of the site to return to normal, etc.

Comment 10

[YF (Yang)]

Further, due to internal cache in Firefox, I could't reproduce it returned to normal website/certificate.

Comment 11

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

(In reply to YF (Yang) from comment #9)

I don't understand what you mean by this last part:

> It will not be cleared, such as when you reload the state, the state of the
> site to return to normal, etc.

Could you provide more concrete steps to reproduce the issue?

(For what it's worth, I am seeing the behavior you mention in comment 10 - I believe this is because of internal DNS caching. One thing we could do is to bypass the DNS cache in the add certificate exception dialog, but from what you're saying I'm assuming there's another bug here?)

Comment 12

[YF (Yang)]

I encountered a fault website before fill this bug. its security certificate changes, but IP does not change.

To reproduce this bug under normal environment, I need to deploy a normal and then fault with multiple error SSL host locally, or through the use of hosts file.

Yes, looks reproduce it by hosts file needs to bypass the DNS cache. It is a small bug for UI update, but it does exist.


I hope you can understand it from the code (link on comment 0), e.g. 'setText("status2Description", "")' when if (!use2), i.e. remove 'if' and set "" for labels.

I guess I can do it now, although make a patch have some trouble for me.

Comment 13

[YF (Yang)]

Sorry, I still encounter some problems for the coding.


resetDialog do something similar, but more thorough.

Comment 14

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

(Back in October I had intended to complete a patch on this but then I stalled since I ran out of energy to write tests. Hopefully sometime soon I'll complete this. In case I don't ever, I'm updating the summary to be more clear about what I was intending to fix. The basic idea is that the dialog will fail in unhelpful ways if given unexpected input (in either the "location" field or if the encountered certificate changes, etc.))