Address reported security issues in the LINCS module#652
Conversation
* DownloadCustomGCTReportAction.getView: validates the requested fileName and confines the resolved path to the container's GCT directory before streaming and deleting it, closing a path-traversal arbitrary file read/delete reachable by any Reader. * LincsManager.getLincsPspJob/getLincsPspJobForRun: requires a Container and filters on it, so PSP-job detail/status/update actions can no longer reach jobs in other folders by guessing an id (cross-container IDOR). Updated all callers. * Requires https for the Clue/PSP server URI, rejects both at save (ManageLincsClueCredentials) and at use (LincsPspUtil.getPspEndpoint), so the API key is never sent in clear text (CWE-319, cleartext transmission of sensitive data). * ManageLincsClueCredentials and CromwellConfigAction: write an audit event (no secret values) when credentials or config change. Identified by a security review of the MacCoss lab modules. Co-Authored-By: Claude <noreply@anthropic.com>
…ired rejection from ManageLincsClueCredentials.validateCommand is shown. * Clarified the comments on the PSP endpoint https check (LincsPspUtil) and the Cromwell config audit event (LincsController). No behavior change.
ankurjuneja
left a comment
ankurjuneja
left a comment
There was a problem hiding this comment.
Thanks @vagisha , here is one finding from claude code review using LK skills that I think is relevant here:
Finding #1 - [Severity: Medium] — Consistency / IDOR gap — LincsManager.deleteLincsPspJobsForRun(long runId)
▎ Issue: The PR added a Container parameter to getLincsPspJobForRun and getLincsPspJob, but deleteLincsPspJobsForRun(long runId) (visible just below in the same file) was left filtering by RunId only, with no container filter.
▎ Why it matters: If this method is ever reachable with an attacker-influenced runId, it would delete PSP jobs across container boundaries — the same class of issue the rest of the PR closes. It's likely only invoked internally on run-deletion events (where the run is already container-scoped), which would make it safe, but that should be confirmed.
▎ Suggestion: Verify the call sites. If any path takes a request-supplied runId, add SimpleFilter.createContainerFilter(container) here too for symmetry.
|
Thanks, @ankurjuneja. |
2 participants
Rationale
A security review of the LINCS module identified six issues, addressed here:
Changes
Path traversal (High) - DownloadCustomGCTReportAction
fileNamewithFileUtil.isAllowedFileNameand confirm the resolved path stays inside the container's GCT directory.Cross-container access (Medium / Low) - PSP job lookups
LincsManager.getLincsPspJob/getLincsPspJobForRunnow require aContainerand filter on it.The PSP-job detail/status/update actions can no longer reach jobs in other folders.
All callers updated (3 controller actions, SubmitPspJobAction, and 2 LincsDataTable display columns).
Cleartext credential transmission (Low, CWE-319) - Clue/PSP server URI
httpsfor the Clue/PSP server URI: rejected at save time inManageLincsClueCredentials.validateCommand,and as defense in depth in
LincsPspUtil.getPspEndpointbefore the API key is sent as a request header.The point-of-use check also blocks endpoints saved as
http://before this fix.<labkey:errors/>tomanageClueCredentials.jspso the save-time rejection is actually shown.Missing audit trail (Low) - credential / config changes
ManageLincsClueCredentials.handlePostandCromwellConfigAction.handlePostnow write an audit event (container and acting user, no secret values) when the PSP/Clue credentials or Cromwell config change.Co-Authored-By: Claude noreply@anthropic.com