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

Avoid CodeQL warning by flipping logic around tenant config lookup #3968

Closed
lmsurpre opened this issue Sep 12, 2022 · 2 comments
Closed

Avoid CodeQL warning by flipping logic around tenant config lookup #3968

lmsurpre opened this issue Sep 12, 2022 · 2 comments
Assignees
Labels
P2 Priority 2 - Should Have technical debt

Comments

@lmsurpre
Copy link
Member

lmsurpre commented Sep 12, 2022

Currently, when a request comes in with an X-FHIR-TENANT-ID header value, we:

  1. validate the value to ensure it doesn't contain unexpected characters (e.g. / or .); then
  2. lookup the tenant configuration for the passed tenantId (filesystem read)

Despite the input validation, CodeQL is flagging that as a potential vulnerability.

We think we can make that warning go away by flipping our processing.

  1. read all the tenant config directory names during startup and store them in a map
  2. when a request comes in for a particular tenant, look it up in the map (vs looking for a directory of that name from filesystem)
@lmsurpre lmsurpre added technical debt P2 Priority 2 - Should Have labels Sep 12, 2022
@lmsurpre
Copy link
Member Author

lmsurpre commented Sep 22, 2022

Suggestion: isolate the changes to FHIRServletContextListener and FHIRRestServletFilter.

you can construct the set/map in the context listener and use something like event.getServletContext().setAttribute(FHIRPersistenceHelper.class.getName(), persistenceHelper);

we're thinking we might need the map so that we're not actually "using the user input" directly when we do the filesystem access.

then get it back out in the servlet filter to verify it tenant id is valid.

you can use FHIRConfiguration.getConfiguredTenants() to get the list of directories.

@lmsurpre
Copy link
Member Author

Confirmed that this change fixed the security warning: https://github.com/LinuxForHealth/FHIR/security/code-scanning/6

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P2 Priority 2 - Should Have technical debt
Projects
None yet
Development

No branches or pull requests

2 participants