[🐛] ngx-cookie-service makes CSRF challenge hard to solve

[dnull]

🐛 Bug report

Description

Sorry for the long description, but I think this is a tricky one...

Current situation

The CSRF Challenge is hard to solve in its current state, as it requires the use of seriously outdated browsers (see issue #1421) for the attack to work. While this was initially thought to be a consequence of changing cookie defaults (SameSite=Lax instead of SameSite=None) advocated by initiatives such as Incrementally Better Cookies (see #1421), the behavior cannot be attributed only to this.

Another explanation

A closer look revealed the following connection to a third-party dependency:

• The CSRF Challenge was included in Juice Shop with PR Add CSRF challenge #1322 and introduced with release 10.1.0 in March 2020.
• The same release switched from the NPM package ngx-cookie to ngx-cookie-service for the handling of cookies in the JavaScript frontend, see commit 1c68c55.
• In the same month, ngx-cookie-service switched the default for new cookies to SameSite=Lax, see stevermeister/ngx-cookie-service@0cb4839.

This resulted in a changed behavior when setting new cookies.

Role of e2e tests

As Google gradually rolled out SameSite=Lax in Chrome during the same time, the failure in the e2e test as discussed in #1421 was attributed to that rollout and not to the changed behavior of ngx-cookie-service.

Current impact on the CSRF challenge

The frontend sets the token cookie using the set method of CookieService: https://github.com/bkimminich/juice-shop/blob/e07059edd3b4ed0527add13d0dd5c8570daa3bcc/frontend/src/app/login/login.component.ts#L78

The optional SameSite parameter is not specified by the frontend, which causes ngx-cookie-service to set it to Lax before passing the call to the cookie API of the browser. The default behavior of the browser does thus not matter, as it is only considered when the value for SameSite is left unspecified.

Overall, the CSRF challenge is thus unnecessarily hard to solve, as it requires very old browser versions which do not have support for the SameSite attribute (e.g. Firefox versions < 60). In comparison, even versions of Firefox as of February 2021 still set cookies with SameSite=None by default and it should thus still be reasonably easy to solve the challenge.

Suggested solution

To make the challenge solvable with more browsers, the frontend should use a cookie library which keeps the SameSite attribute unspecified when setting the token cookie. ngx-cookie-service will always specify a value before calling the browser API and is thus not suitable. Explicitly specifying SameSite=None is not an option, as it also forces the Secure flag to be set, which breaks usage on localhost and other unencrypted deployment scenarios.

Is this a regression?

No. The issue exists since the CSRF challenge was released in Juice Shop 10.1.0.

🔬 Minimal Reproduction

1. Verify that the browser used for the test defaults to SameSite=None, e.g. using https://samesite-sandbox.glitch.me/ - the entry for "Cross-site?" in the first line should say "set".
2. Follow the steps for the documented challenge solution.
3. Observe that the cookie is not sent in the request to the profile page, the challenge is not solved.

🔥 Exception or Error

The exception for missing session cookie is shown.

🌳 Your Environment

Tested with release 10.1.0 and the latest develop branch.

Additional Information

Tested with Firefox 73.3.0esr on Kali Linux.

[bkimminich]

Hands down, this is probably the best bug report I've ever read in my entire professional and open source carreer... 😆

🏁 Looks like we're looking for a slightly less security aware cookie module then! Ideas are welcome.

[chinggg]

Thanks for the excellent bug report and suggestion! You mean if replacing ngx-cookie-service with another cookie library which keeps the SameSite attribute unspecified when setting the token cookie, the problem would be solved ?
I am not familiar with these Web technique details yet, so I could not assign the issue to myself. I am trying to understand the codebase now, if anyone else can solve it, that will also be fine!

[dnull]

[...] You mean if replacing ngx-cookie-service with another cookie library which keeps the SameSite attribute unspecified when setting the token cookie, the problem would be solved ?

Exactly!

I am not familiar with these Web technique details yet, so I could not assign the issue to myself. I am trying to understand the codebase now, if anyone else can solve it, that will also be fine!

At the moment I am not considering to fix it myself, as I have basically no clue about Angular development and no knowledge which other cookie libraries are used in realistic production applications.

[adityaofficial10]

@bkimminich
I think we should use 'js-cookie' here instead of 'ngx-cookie-service'.
I read through its documentation and found that it has no default value of sameSite.
So we just have to replace 'ngx-cookie-service' with 'js-cookie'. Right?
Should I go ahead with the implementation?

[chinggg]

I have been trying to replace ngx-cookie-service with ngx-cookie to see whether ngx-cookie will leave the SameSite attribute unspecified. My approach is to make changes back to 1c68c55 mannually, since I do not know whether there is a tool to automatically make modifications when migrating to another npm package.

[adityaofficial10]

@chinggg
Yeah actually if you're unsure about it.
Then js-cookie is a sure shot as it leaves it unspecified..

[bkimminich]

For me any cookie module is fine that works with Angular. If the syntax changes for the components then this just needs to be migrated along with the PR. So feel free to try out and pick the one with the least obstructing impact.

[adityaofficial10]

Okay @bkimminich!
I'll come up with a PR shortly.

[dnull]

I'll come up with a PR shortly.

Once this is fixed, we also have a chance to re-enable the deactivated e2e test: it is possible to tell Chrome that it should default to SameSite=None using the argument --disable-features=SameSiteByDefaultCookies. I am happy to assist you if want to integrate this directly into your PR, otherwise I can prepare my own after yours is merged.

For my local test, I simply downgraded ngx-cookie-service from version 11.0.2 to 2.4.0. Together with the aforementioned argument in the Protractor configuration, the e2e test for the CSRF challenge then passes successfully. But I still think that switching to another up-to-date library which leaves SameSite unspecified is more realistic than pinning ngx-cookie-service to such an old version.

[adityaofficial10]

@dnull
Thanks..
Once I get done with this..
We'll collaborate for sure...

[chinggg]

I have been trying to migrate from ngx-cookie-service to ngx-cookie since they seem to have much in common, I only need to use grep and sed to change some method names and parameter names. Additionally, I need to import { CookieModule } in *.component.spec.ts and add CookieModule.forRoot(), otherwise it will cause NullInjectionError: No provider for CookieService. These modifications does work for most components except the LocalBackupService. Now when I run npm test, there are only errors that have something to do with LocalBackupService.

NullInjectorError: R3InjectorError(DynamicTestModule)[LocalBackupService -> CookieService -> CookieService]: 
	  NullInjectorError: No provider for CookieService!
	error properties: Object({ ngTempTokenPath: null, ngTokenPath: [ 'LocalBackupService', 'CookieService', 'CookieService' ] })
	NullInjectorError: R3InjectorError(DynamicTestModule)[LocalBackupService -> CookieService -> CookieService]: 
	  NullInjectorError: No provider for CookieService!
	    at NullInjector.get (node_modules/@angular/core/fesm2015/core.js:11045:1)
	    at R3Injector.get (node_modules/@angular/core/fesm2015/core.js:11212:1)
	    at R3Injector.get (node_modules/@angular/core/fesm2015/core.js:11212:1)
	    at injectInjectorOnly (node_modules/@angular/core/fesm2015/core.js:4723:1)
	    at ɵɵinject (node_modules/@angular/core/fesm2015/core.js:4727:1)
	    at Object.LocalBackupService_Factory [as factory] (ng:///LocalBackupService/ɵfac.js:4:46)
	    at R3Injector.hydrate (node_modules/@angular/core/fesm2015/core.js:11380:1)
	    at R3Injector.get (node_modules/@angular/core/fesm2015/core.js:11201:1)
	    at NgModuleRef$1.get (node_modules/@angular/core/fesm2015/core.js:25276:1)
	    at Object.get (node_modules/@angular/core/fesm2015/core.js:24990:1)

I cannot understand why this just does not work for the LocalBackupService

[bkimminich]

LocalBackupService uses the actual CookieService in its test where some other components mock it... maybe that doesn't work similarly with the new module?

[chinggg]

I don't know the difference between the two modules since I cannot read through the whole source code to analysis it. I have opened an issue to ngx-cookie reporting this error, hoping they can answer this.
Btw, when I modify just the LocalBackupService to use ngx-cookie without any changes on other components, the npm test have no errors. That's so strange.

[bkimminich]

Did you try switching that test to use a Jasmine mock instead and did that make any difference? That might be the easiest approach maybe...

[bkimminich]

Did you try switching that test to use a Jasmine mock instead and did that make any difference? That might be the easiest approach maybe...

Any luck with this (or another) approach?

[chinggg]

@bkimminich Sorry, I will have final exams of winter semester in the next two days so I am preparing for the exam now.
Thanks for advising me to switch the test to use a Jasmine mock. I must admit that I did not know what is Jasmine at all back then.
After searching and reading I know it is a BDD test framework that uses describe and it to test the behavior of JavaScript code. But I still have no idea how to swicth the test to use a Jasmine mock exactly, since the test in local-backup.service.spec.ts already uses Jasmine. Maybe there is some difference that I don't know for now.

Btw, I have created an issue to ngx-cookie describing the error, then I received the suggestion like below.

You can import the module, that is the quick solution. But a better way would be refactoring your tests and mocking the CookieService:

describe('MyService', () => {
  // ...
  let service: MyService;
  let cookieServiceSpy: SpyObj<CookieService>;

  beforeEach(waitForAsync(() => {
    // ...
    cookieServiceSpy = jasmine.createSpyObj<CookieService>('CookieService', ['put', 'get']);
    TestBed.configureTestingModule({
      providers: [
        // ...
        {provide: CookieService, useValue: cookieServiceSpy}
      ]
    })
      .compileComponents()
  }));

  it('should set some cookies in some case', () => {
    service.methodThatLeadsToSavingACookie();
    expect(cookieServiceSpy.put).toHaveBeenCalled();
  });
});

And I also found that the errors are not just about LocalBackupService, in fact all of them occur in ScoreBoard component, throw out errors like TypeError: Cannot read property 'ngOnInit' of undefined after NullInjectorErrors.
I have pasted the log here, if you have interests to review it you can click it and go to line 422 where the errors begin to occur.

I will continue to work on it after the exam. And I also have interests in this year's GSoC

[Krshivam25]

hi @bkimminich I would like to work on it. Please suggest me a way to get started.

[chinggg]

@bkimminich I found that these errors are caused byScoreBoardComponent for lack of providers declaration. I added these lines in score-board.component.spec.ts then there are no errors!

+ let localBackupService: any
      providers: [
+        { provide: LocalBackupService, useValue: localBackupService },
      ]

(But why there are no errors when using ngx-cookie-service? Since the errors could be attributed to the lack of LocalBackupService provider declaration.)
But the bad message is the challenge cannot be solve by the new approach 😞

Does the warning The loading of “http://localhost:3000/profile” in a frame is denied by “X-Frame-Options“ directive set to “SAMEORIGIN“ means that ngx-cookie will neither leave the SameSite unspecified?
I will ask the ngx-cookie developer to get the answer. Or another library rather than ngx-cookie should be used.

[dnull]

@chinggg Warning messages related to X-Frame-Options are expected and not a sign that the challenge has failed. The only consequence of this is that the content of the profile page is not allowed to be displayed within a frame of another origin like htmledit.squarefree.com. The HTTP request which manipulates the username in the profile is however still being received and processed by the application (if all conditions are met, like being logged in, SameSite being unspecified and use of a browser that does not default to SameSite=Lax).

I am happy to test your changes, if you point me to a repository and branch which contains them.

[chinggg]

@dnull Thanks! I thought there may be some rookie
mistakes in my changes. You can view my commit at https://github.com/chinggg/juice-shop/tree/ngx-cookie

npm test result is pasted here https://paste.ubuntu.com/p/jnDJVbfWcR/

[dnull]

@chinggg I was successfully able to solve the CSRF challenge using your changes. You should be able to do the same when trying to solve the challenge with Firefox.

Once you are confident that your changes are ready for a pull request (I have only verified that the CSRF is solvable and nothing else), it would be great if you could also re-enable the e2e test for the CSRF challenge, which are located in test/e2e/profileSpec.js. I have already figured out how to change Chrome's default for SameSite during the test run, see #1592 (comment).

[chinggg]

@dnull To change Chrome's default behavior, I add this line in ./protractor.conf.js

  capabilities: {
    browserName: 'chrome',
    proxy: proxy,
    chromeOptions: {
      args: [
        '--window-size=1024,768',
+       '--disable-features=SameSiteByDefaultCookies'
      ]
    }
  },

In test/e2e/profileSpec.ts I replace xit with it and uncommenting the expect lines, then CSRF e2e test passed successfully!

Btw, I have been executing npm run protractor to run e2e tests for three hour, there are some tests saying A Jasmine spec timed out. Resetting the WebDriver Control Flow, you can see the whole log here.
In fact, when I checkout to the upstream version, the same errors occur too. I don't know if it has something to do with my network...GFW or something else
I also found the Score-Board reaaally slow to load, I hope that I can help to improve this in GSoC 2021.

[bkimminich]

Solved via #1612! @dnull and @chinggg please send me your post addresses via mail or Slack DM if you haven't before, then I'll send you some stickers with the next batch I'm bringing to the post office! 👍

[bkimminich]

One last thing: If I remove the first step (install Firefox from 2017) from the CSRF solution (https://pwning.owasp-juice.shop/appendix/solutions.html) then all else works like before for that solution, right?

[chinggg]

@bkimminich I think it works fine like before since the e2e test remains the same and the challange is solved successfully. @dnull can test it again.

And I think the browser used to solve this challenge could be not limited to Firefox since Chrome has the option to --disable-features=SameSiteByDefaultCookies.

[dnull]

One last thing: If I remove the first step (install Firefox from 2017) from the CSRF solution (https://pwning.owasp-juice.shop/appendix/solutions.html) then all else works like before for that solution, right?

The solution does no longer require such an old Firefox version, current Firefox releases should work fine. But when at some point in the future Mozilla follows Google with the new default policy of SameSite=Lax, this will stop to work. Since Juice Shop is all about learning, my suggestion is to add the following aspects to the documentation of the CSRF challenge:

1. Make users aware that browsers behave differently and that this is an ongoing transition.
2. Show how you can check the behavior of your browser using https://samesite-sandbox.glitch.me/.
3. Tell users that Firefox should work out of the box and Chrome can be started using --disable-features=SameSiteByDefaultCookies to restore the old behavior.

I will try to prepare a pull request this week with such an update to the documentation.

[github-actions]

This thread has been automatically locked because it has not had recent activity after it was closed. 🔒 Please open a new issue for regressions or related bugs.