Closed Bug 1370468 Opened 7 years ago Closed 6 years ago

CSP: Write frame-ancestor tests including userpass

Categories

(Core :: DOM: Security, enhancement, P2)

Product:
Core
Component:
DOM: Security
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla60
Tracking Flags:
Tracking Status
firefox60 --- fixed

People

(Reporter: ckerschb, Assigned: vinoth)

References

(Blocks 1 open bug)

Details

(Whiteboard: [domsecurity-active])

Attachments

(1 file, 4 obsolete files)

Bug 1370468 - frame-ancestor tests added for userpass
59 bytes, text/x-review-board-request
ckerschb
: review+
franziskus
: review+
Details 
We should write a testcase to make sure frame-ancestors can not peak at userpass and potentially leak that information through exhaustive pulling.
Blocks: csp-w3c-3
Priority: -- → P3
Whiteboard: [domsecurity-backlog1]
Within this code [1] we make sure that userpass is ignored when performing the frame-ancestors check. The test we should write within this bug should either extend test_frameancestors.html [2] or alternatively we could add a new test to dom/security/test/csp which verifies that userpass is ignored.

Preferrably we add a new test though.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/security/nsCSPContext.cpp#1279-1365
[2] https://dxr.mozilla.org/mozilla-central/source/dom/security/test/csp/test_frameancestors.html
To understand the scenario better, 
I would like to get clarifications on few of my assumptions.

Q1. I assumed Auth URL with userpass will be looking like below,

    https://username:userpassword@www.example.com/webcallback?foo=bar

Correct me if there are any more URL combinations that are possible.

Q2. I check in the new test whether "@" symbol is still in the URL, even though it is ignored in the code.

Q3. Also let me know whether I need to also check scenarios for multiple <iframe> nested in the html.

Thanks.
Flags: needinfo?(ckerschb)
(In reply to Vinothkumar from comment #2)
> Q1. I assumed Auth URL with userpass will be looking like below,
>     https://username:userpassword@www.example.com/webcallback?foo=bar

That is correct; please find our relevant implementation of that part here [1]

> Q2. I check in the new test whether "@" symbol is still in the URL, even
> though it is ignored in the code.

In whatever ways we might potentially leak username or password is not desired. You could use the @ for a first iteration and then we can refine your test.

> Q3. Also let me know whether I need to also check scenarios for multiple
> <iframe> nested in the html.

Yes, I think that might be nice. Ideally you write a new test that can test multiple scenarios at the same time. E.g. multiple iframe nesting, URLs with and without paths and URLs with and without userpass. Here you can see how to add a new test to the testsutie, which you can then run using:
./mach test dom/security/test/csp/test_frame_ancestors_userpass.html

[1] https://dxr.mozilla.org/mozilla-central/source/netwerk/base/nsIURI.idl#9-28
[2] https://hg.mozilla.org/mozilla-central/rev/6c69390e7b8a
Flags: needinfo?(ckerschb)
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #3)
> Here you can see how to add a new test to the testsutie [2]
> [2] https://hg.mozilla.org/mozilla-central/rev/6c69390e7b8a
Here by "exhaustive pulling", you mean leakage of userpass through violation reports to host?(In reply to Christoph Kerschbaumer [:ckerschb] from comment #0)
> We should write a testcase to make sure frame-ancestors can not peak at
> userpass and potentially leak that information through exhaustive pulling.
Files submitted for review are to verify my basic approach for the test.
Let me know if that need to be changed.
(In reply to Vinothkumar from comment #8)
> Files submitted for review are to verify my basic approach for the test.
> Let me know if that need to be changed.

Thanks, there are a few other things I have to take care off - I'll hopefully get to review those files in the afternoon.
Comment on attachment 8902695 [details]
Bug 1370468 - file for Frame-ancestor tests for userpass added

https://reviewboard.mozilla.org/r/174374/#review179976

Hey Vinothkumar,

thanks for taking on the work - you are definitely on the right track. I see a lot of similarities to test_frameancestors though. While that is not bad by itself, it would be better if we don't only replicate that test but specifically focus on userpass for this particular test. Here is my proposal:

* add test_frameancestors_userpass.html which loads file_frameancestors_userpass.html which only loads 2 iframes:
  <iframe src="http://sampleuser:samplepass@mochi.test:8888/.../frame_a.html>
  <iframe src="http://sampleuser:samplepass@mochi.test:8888/.../frame_b.html>
* frame_b.html then could load a nested frame of some sort
* you can use e.g. frame_a.html^headers^ to add a CSP, that way you can eliminate the *.sjs file completely
* and then test_frameancestors_userpass.html can still rely on csp-on-violate-policy observer to test for correct behavior.

I guess what's important to test is the following, have a look at:
> if (topic === "csp-on-violate-policy") {
>      //these were blocked... record that they were blocked
>      window.frameBlocked(asciiSpec, data);
>  }
What's the asciiSpec there? I think that URI is used for CSP reporting as well, and I think within our one testsuite we should make sure that userpass is stripped there as well.

Bonus question, what would happen if the CSP for one file would look like this:
> frame-ancestors sampleuser:samplepass@mochi.test:8888
I suppose our CSP parser is not accepting that, but probably worth testing as well; e.g. we could add a test to TestCSPParser.cpp to make sure our assumptions are correct.
Attachment #8902695 - Flags: review?(ckerschb)
Comment on attachment 8902694 [details]
Bug 1370468 - frame-ancestor tests added for userpass

https://reviewboard.mozilla.org/r/174372/#review179978

Please qfold the changes into one patch, thanks
Attachment #8902694 - Flags: review?(ckerschb)
(In reply to Vinothkumar from comment #12)
> Created attachment 8903649 [details]
> Bug 1370468 - Tests for userpass in frame ancestor added

I have made the changes as you mentioned.

> Bonus question, what would happen if the CSP for one file would look like this:
> frame-ancestors sampleuser:samplepass@mochi.test:8888

Above mentioned scenario stripped out the userpass and returned only 'none'.

All iframe src url and urls in frame ancestors with userpass were removed and test passes.
Let me know your comments.
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #11)
> Comment on attachment 8902694 [details]
> Bug 1370468 - frame-ancestor tests added for userpass
> 
> https://reviewboard.mozilla.org/r/174372/#review179978
> 
> Please qfold the changes into one patch, thanks

Yes this time added all files into one patch.
https://reviewboard.mozilla.org/r/175424/
Comment on attachment 8903649 [details]
Bug 1370468 - Tests for userpass in frame ancestor added

https://reviewboard.mozilla.org/r/175424/#review180758

This is getting really close. Just a few more nits and we are ready to go.

::: dom/security/test/csp/file_frame_a.html:14
(Diff revision 1)
> +
> +    <tt>  IFRAME C</tt><br/>
> +    <iframe src='http://sampleuser:samplepass@mochi.test:8888/tests/dom/security/test/csp/file_frame_c.html'></iframe><br/>
> +
> +  </body>
> +</html>

Please use two space indendation consistenly and remove empty lines and whitespaces, here and elsewhere.

::: dom/security/test/csp/file_frame_b.html:10
(Diff revision 1)
> +          parent.parent.postMessage({call: "frameLoaded", testname: "frame_b", uri: window.location.toString()}, "*");
> +    </script>
> +  </head>
> +  <body>
> +
> +    <tt>  IFRAME D</tt><br/>

this file is named *frame_b.html but here it's named 'IFRAME D' - please make that consistent here and elsewhere.

::: dom/security/test/csp/file_frame_d.html^headers^:1
(Diff revision 1)
> +Content-Security-Policy: default-src 'none'; frame-ancestors http://sampleuser:samplepass@example.com/ ; script-src 'self';

I would prefer if we could add that testcase of:

frame-ancestors http://sampleuser:samplepass@example.com/ 

to the compiled CSP Parser tests - here:
https://dxr.mozilla.org/mozilla-central/source/dom/security/test/gtest/TestCSPParser.cpp#801

::: dom/security/test/csp/mochitest.ini:35
(Diff revision 1)
>    file_evalscript_main_allowed.html^headers^
>    file_frameancestors_main.html
>    file_frameancestors_main.js
>    file_frameancestors.sjs
>    file_frameancestors_userpass.html
> -  file_frameancestors_userpass.js
> +  file_frame_a.html

nit: please prefix the helper files with the same naming as the actual tests:
e.g.
test_frameancestors_userpass.html
then helper files should be named:
file_frameancestors_userpass_frame_a.html
...

::: dom/security/test/csp/test_frameancestors_userpass.html:98
(Diff revision 1)
>  // -- we can't determine *which* frame was blocked, but at least we can count them
>  var frameBlocked = function(uri, policy) {
> -  //ok(true, 'a CSP policy blocked frame from being loaded: ' + policy);
> +
> +  //Check if @ symbol is there in URI or in csp policy.
> +  if(policy.includes('@') || uri.includes('@'))
> +  {

nit: curly braces goes on same line as if and please add a space between if and the opening bracket:
if (...) {

::: dom/security/test/csp/test_frameancestors_userpass.html:101
(Diff revision 1)
> +  //Check if @ symbol is there in URI or in csp policy.
> +  if(policy.includes('@') || uri.includes('@'))
> +  {
> +    ok(false, ' a CSP policy blocked frame from being loaded. But contains userpass. Policy is: ' + policy + ';URI is: ' + uri );
> +  }
> +  else {

nit: else goes on same line as curly brace:
} else {
  ...
}

::: dom/security/test/csp/test_frameancestors_userpass.html:103
(Diff revision 1)
> +  {
> +    ok(false, ' a CSP policy blocked frame from being loaded. But contains userpass. Policy is: ' + policy + ';URI is: ' + uri );
> +  }
> +  else {
> +    ok(true, ' a CSP policy blocked frame from being loaded. Doesn\'t contain userpass. Policy is: ' + policy + ';URI is: ' + uri );
> +  }

nit: lines should respect the 80 char limit here and elsewhere.
Attachment #8903649 - Flags: review?(ckerschb)
Nit: whenever uploading a new patch, please mark the outdated patches as obsolete - thanks!
Assignee: nobody → cegvinoth
Status: NEW → ASSIGNED
Priority: P3 → P2
Whiteboard: [domsecurity-backlog1] → [domsecurity-active]
Attachment #8902695 - Attachment is obsolete: true
Attachment #8902694 - Attachment is obsolete: true
Attachment #8902694 - Attachment is obsolete: true
Attachment #8902695 - Attachment is obsolete: true
Attachment #8903649 - Attachment is obsolete: true
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #15)
> Comment on attachment 8903649 [details]
> Bug 1370468 - Tests for userpass in frame ancestor added
> 
> https://reviewboard.mozilla.org/r/175424/#review180758
> 
> This is getting really close. Just a few more nits and we are ready to go.

Made all the requested changes. Incase any other changes need to be made let me know.
Flags: needinfo?(ckerschb)
(In reply to Vinothkumar from comment #17)
> Created attachment 8904146 [details]
> Bug 1370468 - Files for frame-ancestor tests including userpass added
> 
> Review commit: https://reviewboard.mozilla.org/r/175926/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/175926/

Made old patches obsolete. Above one is the new patch files.
Comment on attachment 8904146 [details]
Bug 1370468 - Files for frame-ancestor tests including userpass added

https://reviewboard.mozilla.org/r/175926/#review180922

Hey, I am slightly confused now, two major questions:
a) Within the mochitest, what's the difference between test frame_a and frame_b? They seem identical to me, shouldn't one check for nested frames?
b) TestCSPParser, does not return the right result for parsing userpass. The Parser should not recognize userpass and hence should convert to the input to 'none', right?

Happy to chat on IRC if I am just missing something.

::: dom/security/test/csp/file_frameancestors_userpass_frame_a.html:5
(Diff revision 1)
> +<html>
> +  <head>
> +    <title>Nested frame</title>
> +    <script>
> +          parent.parent.postMessage({call: "frameLoaded", testname: "frame_a", uri: window.location.toString()}, "*");

nit: 2 space indendation here and elsewhere

::: dom/security/test/gtest/TestCSPParser.cpp:802
(Diff revision 1)
>      { "frame-ancestors http://a.b.c.d.e.f.g.h.i.j.k.l.x.com",
>        "frame-ancestors http://a.b.c.d.e.f.g.h.i.j.k.l.x.com" },
>      { "frame-ancestors https://self.com:34",
>        "frame-ancestors https://self.com:34" },
> +    { "frame-ancestors http://sampleuser:samplepass@example.com",
> +      "frame-ancestors http://sampleuser:samplepass@example.com" },

I thought that should transfer into:
frame-ancestors 'none'.

did you test that?
Attachment #8904146 - Flags: review?(ckerschb)
(In reply to Vinothkumar from comment #18)
> Made all the requested changes. Incase any other changes need to be made let
> me know.

This is really close, I am mainly worried about TestCSPParser, did you run that test?
> ./mach gtest TestCSPParser
Flags: needinfo?(ckerschb)
Attachment #8904146 - Attachment is obsolete: true
Attachment #8902694 - Attachment is obsolete: true
Attachment #8902695 - Attachment is obsolete: true
Attachment #8903649 - Attachment is obsolete: true
Comment on attachment 8904225 [details]
Bug 1370468 - Files for frame-ancestor tests including userpass added and also testcase added in TestCSPParser

https://reviewboard.mozilla.org/r/176004/#review181014

That looks great now - thanks!
Attachment #8904225 - Flags: review?(ckerschb) → review+
Attachment #8904146 - Attachment is obsolete: true
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #21)
> (In reply to Vinothkumar from comment #18)
> > Made all the requested changes. Incase any other changes need to be made let
> > me know.
> 
> This is really close, I am mainly worried about TestCSPParser, did you run
> that test?
> > ./mach gtest TestCSPParser

Ya this time I ran the test. Test returns false if expected result is other than 'None'. 

And if there any other issues let me know.
I guess you have to make sure that all conflicts are marked as resolved. If so, I think Autoland will pick up the change.
Attachment #8904225 - Flags: checkin+
Attachment #8904225 - Flags: checkin+
Keywords: checkin-needed
Mozreview says at least one patch still needs to be reviewed. Please take a look.
Flags: needinfo?(ckerschb)
Keywords: checkin-needed
Comment on attachment 8904225 [details]
Bug 1370468 - Files for frame-ancestor tests including userpass added and also testcase added in TestCSPParser

https://reviewboard.mozilla.org/r/176004/#review220490
should work now (hopefully) - thanks!
Flags: needinfo?(ckerschb)
Keywords: checkin-needed
Attachment #8904225 - Attachment is obsolete: true
Comment on attachment 8902694 [details]
Bug 1370468 - frame-ancestor tests added for userpass

https://reviewboard.mozilla.org/r/174372/#review221980

This already had my r+, but due to some squashing problems it still showed some conflicts - hope that works now. r=me
Attachment #8902694 - Flags: review?(ckerschb) → review+
Attachment #8902694 - Flags: review?(franziskuskiefer)
Comment on attachment 8902694 [details]
Bug 1370468 - frame-ancestor tests added for userpass

https://reviewboard.mozilla.org/r/174372/#review221986
Attachment #8902694 - Flags: review?(franziskuskiefer) → review+
Pushed by franziskuskiefer@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/39716502b1f1
frame-ancestor tests added for userpass r=ckerschb,fkiefer
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/39716502b1f1
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: