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

Attack Review: Integrity of security critical strings #409

Closed
mikesamuel opened this issue Sep 26, 2018 · 15 comments
Closed

Attack Review: Integrity of security critical strings #409

mikesamuel opened this issue Sep 26, 2018 · 15 comments
Labels

Comments

@mikesamuel
Copy link
Contributor

@bmeck @vdeturckheim @erights @MylesBorins

I'm nearing feature completeness on some security relevant Node packages, and would like to organize attack reviews.

These packages all relate to the integrity of messages that leave Node, for example HTML to the browser, SQL to a database, or strings to a shell.

I could use help coming up with attackable demo applications and help defining what constitutes a breach. Once that's done, I need people to attack the system and share any vulnerabilities found.

The packages that are in bounds are:

Contextual escaping for HTML : https://npmjs.com/package/pug-plugin-trusted-types
Safely composable SQL queries : https://npmjs.com/package/safesql
Safely composable Shell strings : https://npmjs.com/package/sh-template-tag
Module Keys : https://npmjs.com/package/module-keys
Contract Type Utilities : https://npmjs.com/package/node-sec-patterns
Web Contract Types : https://npmjs.com/package/web-contract-types

@vdeturckheim
Copy link
Member

Good initiative.
I think we can find a few hackable apps around:

Are any other known?

@bmeck
Copy link
Member

bmeck commented Sep 27, 2018

Not everything itself is a Node application that we can make as an attack surface, however, I really like the idea of popping up a nicer exercise based tutorial if we go through things.

https://rawgit.com/Agoric/SES/master/demo/?dateNow=enabled

Is interesting in part because it is so explanatory (even if needing specialized knowledge a bit) and easy to show to others.

I think some of the big things we are going to have to do is define what a valid attack will be fore these. I don't think mucking with the primordials is something we should really account for usually. All attempts at hygiene within an Agent require Realm isolation or frozen Realms.

@mikesamuel
Copy link
Contributor Author

@vdeturckheim Yeah. I think a target app would be good.

@bmeck Yeah, I think we have to assume good faith on the part of developers. That's pretty fuzzy, but out of bounds I think are custom C++ extensions, abuse of bindings, and attacking builtin modules via primordials. For modules like node-sec-patterns which should initialize early and preserve properties even in the face of hostile JS code, I have tests for the ability of late-running code to breach isolation via primordials.

@mikesamuel
Copy link
Contributor Author

I'm working on a demo app that will tie these together and a script to run it locally so we can craft attacks.

Tentatively,

  • an attacker has a full breach if they can
    • ACE: craft a payload that specifies code to run either server or client side
    • SQL: craft a payload that controls the structure of queries sent to the database
    • SHELL: craft a payload that controls the structure of a string sent to a child_process shell
    • TROJAN: craft a payload that causes a write to any file on PATH
    • APP: craft a payload that violates a documented application security property
    • EXFIL: craft a payload that causes the process to send sensitive data to the file system or via a network send to an endpoint they control or in clear text
      (the app will be setup to run locally for testing so may use http instead of https. This does not constitute clear text for the purposes of this experiment)
  • DOS an attacker has a partial breach if they can craft a payload that denies service to good-faith users. Crashing the app process constitutes DOS.
  • TMPDIR: an attacker has a full breach if they can do any of the above in tandem with the ability to read or write any file under /tmp/
  • CODE: an attacker has a full breach if they can do any of the above in tandem with a code change to the app that is deemed likely to pass code review by a typical application developer
  • PACK: an attacker has a full breach if they can do any of the above in tandem with the ability to read the output of the npm packed tarball that is deployed
  • SIBLING-PROC: an attacker has a partial breach if they can do any of the above in tandem with a process that runs on the same machine as a different unprivileged user
  • FS: an attacker has a partial breach if they can do any of the above in tandem with the ability to write or read a file at another location after application startup
  • ENV: an attacker has a partial breach if they can do any of the above in tandem with control of the environment variables with which node is launched.
  • SMELLY-CODE: an attacker has a partial breach if they can do any of the above in tandem with a code change to the app that seems dodgy but which a typical application developer probably wouldn't be able to explain a risk

The CODE and SMELLY-CODE victory conditions are unfortunately subjective but I've no better way to represent the need for security under maintenance when developers or reviewers are under time pressure.

@mikesamuel
Copy link
Contributor Author

mikesamuel commented Oct 13, 2018

Status update

I've got a skeletal demo-app at github.com/mikesamuel/attack-review-testbed that has an intentionally high attack surface to allow testing the effectiveness of mitigations

  • It uses part of the request URL to require a handler but uses resource integrity hooks to prevent loading of code from untrusted sources
  • It generates HTML from Pug templates but uses pug-require to provide extra mitigations and to produce TrustedHTML values instead of strings, and hooks the server to only append TrustedHTML to the body.
  • It relies on access control checks in SQL generated using template strings but uses safesql to auto-escape those values and wraps the postgres library to check that only trusted SqlFragments reach query
  • It loads jsdom so that it can use DOMPurify to sanitize before render but prevents jsdom from accessing http or child_process via sensitive module hooks.
  • it locks down eval and new Function via V8 flags but re-enables them for modules like promise and depd using new Proxy(Function, ...) tricks.

I still have some work to increase the attack surface. I plan to

  • Allow file upload ostensibly of images, but without explicit checking of polyglot attacks to make it easy to get files that could be required onto the system.
  • Allow upload of tarballs of images so that I can allow attacks against code that uses sh-template-tag to untar uploaded images.
  • Increase the attack surface w.r.t. attacker controlled properties by allowing JSON uploads of image/post metadata, and to do silly things with functions from objects to allow probing of new Function protections.

@mikesamuel
Copy link
Contributor Author

Anyone interested in a weekly call to sort out logistics for the attack review, and how to recruit attackers?

@lirantal
Copy link
Member

@mikesamuel it looks interesting, thanks for sharing!

I edited your post above to correct the link to github and I'm suggesting that you maybe copy the information you added in this thread comments (1, 2), to a README on the project?

@mikesamuel
Copy link
Contributor Author

@lirantal Thanks for the fix.

maybe copy the information you added in this thread comments (1, 2), to a README on the project

Will do. I'm almost feature complete, then I'll focus on getting those disparate pieces of documentation organized.

@mikesamuel
Copy link
Contributor Author

Status Update

As @lirantal mentioned, there's a draft README and a breach report template.

The pre-launch issue tracker labels collects tasks that should be completed before inviting attack.

I have a line on some attack reviewers, but would appreciate suggestions on where to recruit more.

@MarcinHoppe
Copy link
Contributor

@mikesamuel I think we can use the Security WG Slack workspace to host a channel dedicated to this effort. I can spread the word among my team mates, they love a good challenge.

@mikesamuel
Copy link
Contributor Author

Thanks @MarcinHoppe

I created a channel on https://nodejs-security-wg.herokuapp.com/ and I changed the support links from

Getting Answers To Questions

TODO: Slack channel or something similar

to

Getting Answers To Questions

Post on the # attack-review channel at
https://nodejs-security-wg.slack.com/ if you're having trouble.

We'll try to update the wiki in response to common questions.

If you need a private channel:

Contact Availability DMs
Mike Samuel US EST (GMT+5) twitter/@mvsamuel

@mikesamuel
Copy link
Contributor Author

Status Update

@MarcinHoppe @lirantal @deian
I think it's ready for early attackers to at least iron out any bugs in the getting started discussions.
I'll be on the #attack-review slack channel in case anyone wants to chat.
Next I'll be putting together a short screencast video walking through the target app.

Recent Changes

  • Implemented a vulnerable variant of the server.

    Testing Attacks against a vulnerable server

    Running scripts/build-vulnerable.sh will copy the server source files over to a directory vulnerable/.

    You can then run vulnerable/scripts/run-locally.js to start up the modified server.

    The vulnerable server has most of the mitigations disabled, so you can try an attack against the vulnerable server. If it doesn't work against the target server, then the difference between the two mitigated the attack.

  • Wrote a post-attack review questionairre so we can collect overall impressions from attackers.

  • Fixed a number of bugs:

    • Post visibility logic was too complex around friend relationships and probably broken.
    • "View as public" was broken. Usability: "view as public" banner did not show up on page.
    • Usability: there was no way to get to the post form from the home page.
    • "Getting started" instructions didn't work when user had no ambient git credentials. Now uses https link.

@lirantal
Copy link
Member

Seems it will be quite some undertaking of manual work to pentest the app, but the concept of a security-hardened Node.js is intriguing and I think that it is interesting to see how this will work with Anna's security policies PR as well as possibly @vdeturckheim efforts of fuzzing Node.js core to help with this test-bed.

@mikesamuel
Copy link
Contributor Author

@lirantal

Seems it will be quite some undertaking of manual work to pentest the app, but the concept of a security-hardened Node.js is intriguing

I think if I can get enough eyes on this, we can get coverage without any one person having to test the whole thing.

and I think that it is interesting to see how this will work with Anna's security policies PR as well as possibly @vdeturckheim efforts of fuzzing Node.js core to help with this test-bed.

@addaleax
There is a vulnerable variant of the server that has mitigations removed. That could serve as the basis for a target app with Anna's policies.

IIUC, Anna's policies focus on controlling what process-external endpoints can be connected to, not the integrity of messages sent to those endpoints. If that's the case, then the message-integrity preserving goals of this could complement Anna's policies.

This attack review focuses on:

  1. XSS: Arbitrary code execution client-side.
  2. Server-side arbitrary code execution
  3. CSRF: Sending state-changing requests that carry user credentials without user interaction or expressed user intent.
  4. Shell Injection: Structure of shell command that includes crafted substrings does not match developers' intent.
  5. SQL Injection: Structure of database query that includes crafted substrings does not match developers' intent.

IIUC, Anna's work is relevant to (2, 4, 5) but its goals do not include addressing (1, 3).

@github-actions
Copy link
Contributor

This issue is stale because it has been open many days with no activity. It will be closed soon unless the stale label is removed or a comment is made.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants