Skip to content
New issue

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

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

Already on GitHub? # to your account

[imaging_browser] Escape the header table in view session page #7552

Merged
merged 1 commit into from
Sep 24, 2021

Conversation

driusan
Copy link
Collaborator

@driusan driusan commented Aug 26, 2021

The header table in the imaging_browser viewSession page is generated
by smarty, but the variables are not properly escaped. This adds the
|escape filter to all the outputs to fix reflected XSS attacks on that page.
(In particular, the outputType which comes from the URL, but all variables
are HTML escaped in this PR just to be on the safe side.)

The header table in the imaging_browser viewSession page is generated
by smarty, but the variables are not properly escaped. This adds the
`|escape` filter to all the outputs to fix reflected XSS attacks.
@driusan driusan added Bug PR or issue introducing/requiring bug fixes (not mutually exclusive with the Feature label) Security PR patches a vulnerability, makes resource access changes, or updates dependencies labels Aug 26, 2021
Copy link
Collaborator

@ridz1208 ridz1208 left a comment

Choose a reason for hiding this comment

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

@driusan approved in theory, but I'm curious to know from @jesscall or @cmadjar if this affects functionality in any way.

@driusan
Copy link
Collaborator Author

driusan commented Aug 26, 2021

It shouldn't, DoB/Sex/QC Status/Visit Label/etc shouldn't have any HTML in them, but I'll wait for confirmation before merging..

@driusan
Copy link
Collaborator Author

driusan commented Aug 27, 2021

@jesscall @cmadjar can you confirm that this isn't going to break anything?

@jesscall
Copy link
Contributor

jesscall commented Aug 27, 2021

Looks good

Screen Shot 2021-08-27 at 10 20 49 AM

I agree with @driusan, there shouldn't be any HTML in these vars. Curious to know however what would happen if there is a special character in any of the variables (& " ' < > *). For example, the scanner name is not set by us / a developer.

Is there an example on RB somewhere of the outputType being set? I'm not familiar where that comes from and would be interested in testing that aspect based on this comment:

to fix reflected XSS attacks on that page.
(In particular, the outputType which comes from the URL

@driusan
Copy link
Collaborator Author

driusan commented Aug 27, 2021

The outputType comes from the URL, that's why this is a security problem that needs to be fixed.

@jesscall
Copy link
Contributor

@driusan yes, but from what I've seen outputType in the header table is always empty. In what case is a value actually displayed from the URL? What does it correspond to ?

@jesscall
Copy link
Contributor

NVM, realized if you choose anything other than all types on imaging browser it is filled out (defaced, native)

@jesscall jesscall added the Passed Manual Tests PR has undergone proper testing by at least one peer label Aug 27, 2021
@driusan driusan added the Critical to release PR or issue is key for the release to which it has been assigned label Sep 14, 2021
@driusan driusan merged commit 406b550 into aces:23.0-release Sep 24, 2021
@ridz1208 ridz1208 added this to the 23.0.7 milestone Sep 30, 2021
@ridz1208 ridz1208 modified the milestones: 23.0.7, 23.0.8 Oct 19, 2021
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Bug PR or issue introducing/requiring bug fixes (not mutually exclusive with the Feature label) Critical to release PR or issue is key for the release to which it has been assigned Passed Manual Tests PR has undergone proper testing by at least one peer Security PR patches a vulnerability, makes resource access changes, or updates dependencies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants