Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JavaScript generation leads to significant security hole on the client side #4340

Closed
pond opened this issue Mar 6, 2019 · 4 comments

Comments

Projects
None yet
5 participants
@pond
Copy link
Contributor

commented Mar 6, 2019

  • ABP 4.2
  • .Net Core
  • No exception message / stack trace applies

We have had some security checking work done on our ABP application. This showed up a vulnerability with the JavaScript generation in e.g. LocalizationScriptManager.cs.

In this particular case, the tester had bypassed the GUI validation for defining new languages as a tenant admin. It seems there's not much validation on the server side, so they were able to add very scrambled arbitrary display names for languages such as foo''bar.

The JavaScript generator compiles this with no escaping at all - in fact that's true of a lot of the scripts; they just add strings together, including stuff which comes from database quantities where those quantities are user-editable. This is exceptionally dangerous.

In this particular case, the auto-generated script was simply syntactically invalid:

// ...
{
        name: '4wbh''yenx',
        displayName: '4wbh''yenx',
        icon: 'famfamfam-flags ag',
        isDisabled: false,
        isDefault: false
}
// ...

...and so the JS layer suffered a parsing fault. This cripples the GUI, and it becomes impossible for anyone to use the system. Even the tenant admin can't fix it at the GUI, because the GUI relies too much on JS to be able to get that far. Further, trying to tidy up at the database is still problematic as there are layers of cacheing involved which keep serving the broken JS file and you can't get to the maintenance GUI to clear those caches since the GUI is broken.

  1. Needs more strict server-side validation on the back-end behind language management.
  2. Any and all server-side generated JS content must escape values compiled into strings, in exactly the same way as one must escape data injected into HTML content generated at the server.

I haven't got a patch for this as (2) seems such a widespread problem that it clearly requires a rather fundamental structural change to the entire JS generation model inside ABP.

@maliming

This comment has been minimized.

Copy link
Member

commented Mar 6, 2019

2.Any and all server-side generated JS content must escape values compiled into strings, in exactly the same way as one must escape data injected into HTML content generated at the server.

I agree.

1.Needs more strict server-side validation on the back-end behind language management.

I think this requires you to verify it yourself.

@pond

This comment has been minimized.

Copy link
Contributor Author

commented Mar 6, 2019

Fair enough.

Credit to Jesse Whitham (@whithajess) & ZX Security NZ (https://zxsecurity.co.nz) for having hit this bump in the road - all I did was dug down the code layers to find the source of it.

@maliming maliming self-assigned this Mar 6, 2019

@ebicoglu

This comment has been minimized.

Copy link
Contributor

commented Mar 6, 2019

thank you @pond your determinations are correct. so there's a need to validate strictly on server and also escape JS chars.

@ismcagdas ismcagdas added this to the Backlog milestone Mar 6, 2019

alirizaadiyahsi added a commit that referenced this issue Mar 21, 2019

Merge pull request #4345 from maliming/EscapeJS
fix #4340 Escaped generated JavaScript.

@hikalkan hikalkan modified the milestones: Backlog, v4.5 Mar 25, 2019

@hikalkan hikalkan removed the priority:high label Mar 25, 2019

@pond

This comment has been minimized.

Copy link
Contributor Author

commented Apr 4, 2019

Thanks. We've upgraded to 4.5 so this fix is now incorporated into our application :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.