Skip to content

Add cds utils modelling #206

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Add cds utils modelling #206

wants to merge 2 commits into from

Conversation

knewbury01
Copy link
Contributor

What This PR Contributes

This PR overall contributes models for the cds.utils CAP module, the implementation of which can be found here and the documentation here. Specifically the model is comprised of :

  • sinks - functionality of the module that allows for file system manipulation (e.g. controlling which file is being read from or controlling which file is removed)
  • additional flow steps - functionality of the module that take a potentially user controlled value and return some representation of that value that is sufficiently unaltered that a path traversal attack could occur

Future Works

The next step of this work will be to include a query that uses CAP specific sources and these sinks to describe the specific potential path traversal vulnerability that could occur through this API

and test for model
@jeongsoolee09
Copy link
Contributor

Good work! I'd like to suggest some changes that aren't tied to specific locations, so I'm instead putting them here:

Organization of APIs & nomenclature

I first thought of mirroring what was done in the standard library, to first organize then into

  • abstract class FileSystemAccess (defined in Concepts.qll)
    • abstract class FileSystemReadAccess (defined in Concepts.qll)
      • class CAPUtilsFileSystemAccessRead
    • abstract class FileSystemWriteAccess (defined in Concepts.qll)
      • class CAPUtilsFileSystemAccessWrite

But this won't do justice to some of the APIs like cds.util.isdir, cds.util.isfile, and cds.util.local. So I suggest doing:

  1. PathConverters: decodeURI, decodeURIComponent, local
  2. PathPredicates: isdir, isfile
    • BooleanPredicates: exists
  3. DirectoryReaders: find, stat, readdir
  4. DirectoryWriters: rmdir, rimraf, rm, mkdirp
  5. FileReaders: read
  6. FileWriters: write
  7. FileReaderWriters: copy

and not extending any of the standard library classes from Concepts.qll (otherwise we'd get duplicate alerts).

Why make it more fine-grained?

The APIs in the DirectoryReaders, FileReaders, and FileReaderWriters category are not sinks themselves, rather they are taint propagators in that the data read from these functions are tainted given the dependencies of the function calls are tainted. The real vulnerability happens when these data hit a side-effecting function like ones that send an HTTP response (for a close analogy, see UI5LogsToHttp.ql that finds tainted log entries hitting an HTTP response function). We'd like to mark these as stateful taint propagating functions for stateful JavaScript configurations, and having to name them in bulk by their name is obviously convenient.

Location of Sources / Sinks / FlowSteps

These are better suited for query libraries as what counts as one of these three depend on the type of the vulnerability. So, I suggest making a CAPPathInjectionQuery.qll and placing these there (as we're going to write it anyways).

Suggestions on Sink Categorization

  1. SrcDstSink: It's better to break these down into WrittenData and WrittenPath that can be accessed from FileWriters or DirectoryWriters class above.
  2. SimpleSinks, SimpleAdditionalFlowSteps: Dropping the "Simple" naming is suggested since this does not functionally describe a group of APIs. I suggest using functional descriptions such as WrittenData or WrittenPath above.

Suggested future work

cds.utils makes things complicated since it "relays" some libraries by importing and exposing them as its own submodule. If we evaluate cds.utils in the cds repl, we get:

$ npx cds repl
Welcome to cds repl v 9.1.0
> cds.utils
<ref *3> {
  ...
  appendFile: [Function: appendFile],
  appendFileSync: [Function: appendFileSync],
  access: [Function: access],
  accessSync: [Function: accessSync],
  chown: [Function: chown],
  chownSync: [Function: chownSync],
  chmod: [Function: chmod],
  chmodSync: [Function: chmodSync],
  close: [Function: close],
  closeSync: [Function: closeSync],
  copyFile: [Function: copyFile],
  copyFileSync: [Function: copyFileSync],
  cp: [Function: cp],
  cpSync: [Function: cpSync],
  ...
}

...which are borrowed from "fs". Other than these, there are yaml, csv, and tar which we might want to have.

So I suggest adding Model borrowed libraries from standard or third party libraries, namely "fs", "csv", and so on in the future works.

Copy link
Contributor

@jeongsoolee09 jeongsoolee09 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left first comment above.

@jeongsoolee09
Copy link
Contributor

Sorry, please disregard the section Why make it more fine-grained? above, reading from a attacker-controlled path itself constitutes a path traversal vulnerability. However, I still believe the above categorization is valuable.

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

Successfully merging this pull request may close these issues.

2 participants