168629 - Mozilla incorrectly prompts about submitting to an insecure location from a secure one with base href="https://..."

Status: RESOLVED FIXED

(Core Graveyard :: Security: UI, defect)

Description

[Scott Gifford]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.2a) Gecko/20020910
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.2a) Gecko/20020910

When visiting:

   http://redhat.rsc03.net/servlet/website/ResponseForm?koE7iHJoL8kHgKzNkOLRzLimVTTV

and submitting the form, Mozilla warns me that:

    "Although this page is encrypted, the information you have entered is to be
sent over an unencrypted connection and could easily be read by a third party. 
Are you sure you want to continue sending this information?"


It's clear from the URL, and the Page Info and padlock icon agree, that the page
I'm looking at isn't a secure page.

Reproducible: Always

Steps to Reproduce:
1. Visit:
  http://redhat.rsc03.net/servlet/website/ResponseForm?koE7iHJoL8kHgKzNkOLRzLimVTTV

2. Scroll to bottom of page and hit Submit
3.

Actual Results:  
The dialog described above comes up.


Expected Results:  
Normal page submission.

Comment 1

[Boris Zbarsky [:bzbarsky]]

confirming; over to PSM.

Comment 2

[John Unruh]

Evangelism. The same behavior occurs with Nav. 4.7X. It looks like the top line 
- <base href="https://www.redhat.com"> needs to synch up with some lines further 
down -
<form method="POST"
 action="http://redhat.rsc03.net/servlet/campaignrespondent">	

Comment 3

[Scott Gifford]

Although the combination of <base href="https:..."> and <form action="http:...">
is certainly bizarre, I don't think it's illegal, and the error message
displayed by Mozilla is factually incorrect.

We loaded a standard HTTP page, and submitted data to a standard HTTP page.  The
<base href="..."> shouldn't enter into things at all.

I don't mind evangelizing RedHat, but I still think this is a bug in Mozilla. 
I'll move it back into the PSM component in a few days, unless somebody comes up
with a good reason why this behavior is correct.

Comment 4

[Scott Gifford]

See comment #3.

Comment 5

[Scott Gifford]

Attachment: Demo

Comment 6

[Scott Gifford]

Original Web site no longer exists, but unfortunately the bug still does.  I've
created a small test case and attached it.

Comment 7

[Scott Gifford]

Bug is still present in FireBird 20030703.

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

reassign former PSM engineers' bugs to [e-mail]

Comment 9

[Kai Engert [:KaiE:]]

new testcase at URL (as bugzilla meanwhile uses https to display its content)

Comment 10

[:Gavin Sharp [email: [e-mail]]]

*** Bug 328295 has been marked as a duplicate of this bug. ***

Comment 11

[Boris Zbarsky [:bzbarsky]]

So... Kai, you have blame for this.  Why is this using the baseURI of the document?  Shouldn't this use the documentURI or better yet the URI of the document's principal?  Using the baseURI here is bogus, imo.

Comment 12

[Kai Engert [:KaiE:]]

We correctly warn that information will be submitted to an insecure destination, so I don't see this bug as critical.

> So... Kai, you have blame for this.  Why is this using the baseURI of the 
> document?  

I don't know, I didn't write it, I probably just moved this code to a new file, if cvs blames me. But I'm ok to work on it.


> Using the baseURI here is bogus, imo.

I agree.


> Shouldn't this use the documentURI 

I confirm changing
  formURL = document->GetBaseURI();
to
  formURL = document->GetDocumentURI();
fixes the bug.


> or better yet the URI of the document's principal?  

Instead of the above, I also tried
  nsIPrincipal *principal = formNode->GetNodePrincipal();
  principal->GetURI(formURL);
and confirm it fixes the bug, too.

However, documentation says, the principal for a node might be null, "in that case the caller should assume that all same-origin checks against this node fail and that this node has no permissions to do anything."

What should we do if the principal is null?
Cancel submit? (which might break sites?)
Or fall back to document URI?

Comment 13

[Boris Zbarsky [:bzbarsky]]

The only way for a node to have a null principal is if the document it was in has already gone away or if there was an out-of-memory error creating a principal object.  So cancelling the submit is the way to go, in my opinion.

On trunk we're going to change things so a node's principal is never null, fwiw.

Also, system principals have no URI; that might need to be accounted for.

Comment 14

[Kai Engert [:KaiE:]]

> Also, system principals have no URI; that might need to be accounted for.

What does that mean in the context of this bug?

Comment 15

[Boris Zbarsky [:bzbarsky]]

That formURL would be null (if the form post is happening in a chrome document).  No idea what you want the behavior to be in that case.

Comment 16

[Kai Engert [:KaiE:]]

> The only way for a node to have a null principal is if the document it was in
> has already gone away or if there was an out-of-memory error creating a
> principal object.  So cancelling the submit is the way to go, in my opinion.

If that's the only cases, I agree.


> System principals have no URI if the form post is happening in a chrome
> document.  No idea what you want the behavior to be in that case.

I think we should not change current behaviour in that case,
and fall back to the document URI.

Comment 17

[Kai Engert [:KaiE:]]

Attachment: Patch v1

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 213476 [details] [diff] [review]
Patch v1

Move the "||" up to the previous line, and sr=bzbarsky

Comment 19

[Kai Engert [:KaiE:]]

Attachment: Patch v1 b

Addressing review request, moving forward sr=bzbarsky

Bob, could you please do the peer review?

Comment 20

[Robert Relyea]

Comment on attachment 213482 [details] [diff] [review]
Patch v1 b

r=relyea

code implements the recommendation discussed in the bug.

Comment 21

[Kai Engert [:KaiE:]]

Comment on attachment 213482 [details] [diff] [review]
Patch v1 b

There is no nsIContent::GetNodePrincipal on MOZILLA_1_8_BRANCH, we need a different patch if we want to have a fix on that branch, too

Comment 22

[Kai Engert [:KaiE:]]

For the trunk patch we used
  nsIPrincipal *principal = formNode->GetNodePrincipal();

For 1.8 branch, can we use this?
  nsIPrincipal *principal = document->GetPrincipal();

(marking fixed on trunk)

Comment 23

[Boris Zbarsky [:bzbarsky]]

> For 1.8 branch, can we use this?
>  nsIPrincipal *principal = document->GetPrincipal();

Can and should.  ;)

Comment 24

[Kai Engert [:KaiE:]]

Attachment: Patch for 1.8 branch

The only difference between the trunk patch and this one is the line that Boris approved in comment 23, therefore I'm marking it as sr=bzbarsky

Approving for 1.8 branch.

Comment 25

[Kai Engert [:KaiE:]]

checked in to 1.8 branch

Comment 26

[Daniel Veditz [:dveditz]]

Too late for a non-critical bug in 1.7.13, will consider for the next release. Does this not affect Firefox as well?

Comment 27

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #26)
> Does this not affect Firefox as well?

This does affect Firefox.