fix: the file list escapes the name it renders - #869
Merged
Conversation
An account's attachments are listed with the file name the browser supplied, and that
name is stored exactly as it arrived — deliberately, and the upload controller says so:
escaping on the way in stored the entities, so an attachment called "Q&A.txt" was saved
and downloaded as "Q&A.txt". The places that use it are each meant to handle it.
This one did not.
printf('%s (%d KB)', Html::truncate($file->getName() ?? '', 50), ...)
Html::truncate() truncates; it does not escape. The two title attributes on the lines
either side of it are correctly wrapped in $_e(). The response is PLAIN_TEXT and the
front end injects it with jQuery's .html(), so a file named with a script tag runs for
anyone who opens the account — and listing a file needs only view access, not edit, so
the reader is any of the people the account is shared with. On the same page a password
custom field's decrypted value sits in a data-pass attribute, which is what a payload
would go looking for.
The reason it survived is the more useful half of this change. ThemeEscapesWhatItRenders
looks for `<?php echo`, `<?=` and `print`, and its pattern requires whitespace after the
word — `printf(` has none, so every printf in every template was invisible to it. The
pattern now has a branch for printf, and switching it on found nine more places across
eight templates, all fixed here:
- itemshow/item_preset-session_timeout.inc — an administrator's own IP preset
- _partials/fixed-header.inc and _partials/footer.inc — the signed-in user's group name
- install/index.inc and _layouts/main.inc — the application name and version
- configManager/info.inc — the download rate
- wiki/wikipage.inc, twice — page names and URLs (unrouted code, escaped anyway rather
than left as the one exception)
None of those are as reachable as the first, but the point of the check is that it does
not depend on someone deciding which ones matter.
Checked by putting the truncate back unescaped: the guard fails, naming the file, the
line and the expression.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
An account's attachments are listed with the file name the browser supplied, and that
name is stored exactly as it arrived — deliberately, and the upload controller says so:
escaping on the way in stored the entities, so an attachment called "Q&A.txt" was saved
and downloaded as "Q&A.txt". The places that use it are each meant to handle it.
This one did not.
Html::truncate() truncates; it does not escape. The two title attributes on the lines
either side of it are correctly wrapped in $_e(). The response is PLAIN_TEXT and the
front end injects it with jQuery's .html(), so a file named with a script tag runs for
anyone who opens the account — and listing a file needs only view access, not edit, so
the reader is any of the people the account is shared with. On the same page a password
custom field's decrypted value sits in a data-pass attribute, which is what a payload
would go looking for.
The reason it survived is the more useful half of this change. ThemeEscapesWhatItRenders
looks for
<?php echo,<?=andprint, and its pattern requires whitespace after theword —
printf(has none, so every printf in every template was invisible to it. Thepattern now has a branch for printf, and switching it on found nine more places across
eight templates, all fixed here:
than left as the one exception)
None of those are as reachable as the first, but the point of the check is that it does
not depend on someone deciding which ones matter.
Checked by putting the truncate back unescaped: the guard fails, naming the file, the
line and the expression.