table header in output of difflib.HtmlDiff.make_table is not escaped and can be rendered as code in the browser

@larryhastings

BPO	35603
Nosy	@larryhastings, @ned-deily, @serhiy-storchaka, @JulienPalard, @miss-islington, @tirkarthi
PRs	bpo-35603: Escape table header of make_table output that can cause potential XSS #11341 bpo-35603: Escape table header of make_table output that can cause potential XSS #11341 bpo-35603: Escape table header of make_table output that can cause potential XSS #11341 [3.7] bpo-35603: Escape table header of make_table output that can cause potential XSS. (GH-11341) #11353 [3.7] bpo-35603: Escape table header of make_table output that can cause potential XSS. (GH-11341) #11353 [3.6] bpo-35603: Escape table header of make_table output that can cause potential XSS. (GH-11341) #11354 [3.6] bpo-35603: Escape table header of make_table output that can cause potential XSS. (GH-11341) #11354 Revert "bpo-35603: Escape table header of make_table output that can cause potential XSS" #11356 Revert "bpo-35603: Escape table header of make_table output that can cause potential XSS" #11356 Revert "bpo-35603: Escape table header of make_table output that can cause potential XSS" #11356 bpo-35603: Add a note on difflib table header interpreted as HTML #11439 bpo-35603: Add a note on difflib table header interpreted as HTML #11439 bpo-35603: Add a note on difflib table header interpreted as HTML #11439 [3.8] bpo-35603: Add a note on difflib table header interpreted as HTML (GH-11439) #15922

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2019-09-22.06:30:45.659>
created_at = <Date 2018-12-28.09:18:00.849>
labels = ['type-security', '3.7', '3.8', 'docs']
title = 'table header in output of difflib.HtmlDiff.make_table is not escaped and can be rendered as code in the browser'
updated_at = <Date 2019-09-22.06:30:45.652>
user = 'https://github.com/tirkarthi'

bugs.python.org fields:

activity = <Date 2019-09-22.06:30:45.652>
actor = 'xtreak'
assignee = 'docs@python'
closed = True
closed_date = <Date 2019-09-22.06:30:45.659>
closer = 'xtreak'
components = ['Documentation']
creation = <Date 2018-12-28.09:18:00.849>
creator = 'xtreak'
dependencies = []
files = []
hgrepos = []
issue_num = 35603
keywords = ['patch', 'patch', 'patch']
message_count = 12.0
messages = ['332648', '332707', '332708', '332709', '332722', '332724', '332872', '333069', '333072', '351827', '351839', '352962']
nosy_count = 7.0
nosy_names = ['larry', 'ned.deily', 'docs@python', 'serhiy.storchaka', 'mdk', 'miss-islington', 'xtreak']
pr_nums = ['11341', '11341', '11341', '11353', '11353', '11354', '11354', '11356', '11356', '11356', '11439', '11439', '11439', '15922']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'security'
url = 'https://bugs.python.org/issue35603'
versions = ['Python 2.7', 'Python 3.7', 'Python 3.8']

HtmlDiff.make_table takes fromdesc and todesc that are not escaped causing problems while rendering html when they contain tags like fromdesc="<from>", todesc="<to>". There is no validation for them to be filenames so they could be arbitrary strings. Since contents of the table are escaped I think it's good to escape headers too since they might lead to the browser to execute the headers as code and potential XSS. I don't think it's worthy of adding security type so I am adding behavior. Feel free to change the type if needed.

I could see no test failures on applying my patch and I will push a PR with a test.

Current output : (<from> and <to> are not escaped in the output)

$ python3 -c 'import difflib; print(difflib.HtmlDiff().make_table(["<a> hello </a>"], ["<b> hello </b>"], fromdesc="<from>", todesc="<to>"))'

<table class="diff" id="difflib_chg_to0__top"
       cellspacing="0" cellpadding="0" rules="groups" >
    <colgroup></colgroup> <colgroup></colgroup> <colgroup></colgroup>
    <colgroup></colgroup> <colgroup></colgroup> <colgroup></colgroup>
    <thead><tr><th class="diff_next"><br /></th><th colspan="2" class="diff_header"><from></th><th class="diff_next"><br /></th><th colspan="2" class="diff_header"><to></th></tr></thead>
    <tbody>
        <tr><td class="diff_next" id="difflib_chg_to0__0"><a href="#difflib_chg_to0__top">t</a></td><td class="diff_header" id="from0_1">1</td><td nowrap="nowrap">&lt;<span class="diff_chg">a</span>&gt;&nbsp;hello&nbsp;&lt;/<span class="diff_chg">a</span>&gt;</td><td class="diff_next"><a href="#difflib_chg_to0__top">t</a></td><td class="diff_header" id="to0_1">1</td><td nowrap="nowrap">&lt;<span class="diff_chg">b</span>&gt;&nbsp;hello&nbsp;&lt;/<span class="diff_chg">b</span>&gt;</td></tr>
    </tbody>
</table>

New changeset 78de011 by Serhiy Storchaka (Xtreak) in branch 'master':
bpo-35603: Escape table header of make_table output that can cause potential XSS. (GH-11341)
78de011

Is 2.7 affected?

This looks like potentially security issue, so it may be worth to backport the fix to 3.6 and 3.5.

Yes, 2.7 is also affected at

cpython/Lib/difflib.py

Line 2001 in befe3f7

'<th colspan="2" class="diff_header">%s</th>' % fromdesc,

. I think it was missed during the initial addition in bpo-914575 and no one would be using it as a feature to have custom HTML directly injected in table header using fromdesc and todesc. So I hope this won't be breaking anyone on 2.7 and even with that headers can still be customized with CSS. Also as an additional note HtmlDiff.make_file internally uses make_table to construct full HTML file and that is also fixed. Maybe if one of the original authors of difflib would like to confirm it then it would be great.

After some thoughts, I'm no longer sure that this is a correct change. The user can use this for highlighting the part of the column header, adding links or images, displaying complex multiline headers. I suspect that this change will break some existing code that uses difflib.HtmlDiff.make_table() properly, passing escaped strings.

I think now that this change should be reverted, and we should document instead that "fromdesc" and "todesc" are interpreted as an HTML text, and user data should be properly escaped for using in headers.

Thanks Serhiy for the input. I initially thought this should be escaped since content was escaped and the same for header since user input taken directly could result in XSS. Maybe someone might using this undocumented feature intentionally that might not be worth breaking.

I will make a PR for this to be noted in docs that the parameters are interpreted as HTML.

New changeset 830ddc7 by Serhiy Storchaka in branch 'master':
Revert "bpo-35603: Escape table header of make_table output that can cause potential XSS. (GH-11341)" (GH-11356)
830ddc7

Yes, please make a documentation PR. Since this behavior can cause a security hole in the user code, make this note visually attractive (maybe use the "note" directive).

I have added a doc note under make_file docs which has docs for fromdesc and todesc. make_table refers to make_file for fromdesc and todesc docs. I have added a screenshot of the rendering using note and warning directive. I feel note directive is sufficient for this. Placement and wording suggestions are welcome.

New changeset c78dae8 by Julien Palard (Xtreak) in branch 'master':
bpo-35603: Add a note on difflib table header interpreted as HTML (GH-11439)
c78dae8

New changeset 44e36e8 by Miss Islington (bot) in branch '3.8':
bpo-35603: Add a note on difflib table header interpreted as HTML (GH-11439)
44e36e8

Closing this as the docs are merged with the initial patch to escape header reverted. Thanks all for the reviews.

Navigation Menu

Repository navigation

table header in output of difflib.HtmlDiff.make_table is not escaped and can be rendered as code in the browser #79784

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

table header in output of difflib.HtmlDiff.make_table is not escaped and can be rendered as code in the browser #79784

Description

Activity

tirkarthi commented on Dec 28, 2018

serhiy-storchaka commented on Dec 29, 2018

serhiy-storchaka commented on Dec 29, 2018

tirkarthi commented on Dec 29, 2018

serhiy-storchaka commented on Dec 29, 2018

tirkarthi commented on Dec 29, 2018

serhiy-storchaka commented on Jan 2, 2019

serhiy-storchaka commented on Jan 5, 2019

tirkarthi commented on Jan 5, 2019

JulienPalard commented on Sep 11, 2019

miss-islington commented on Sep 11, 2019

tirkarthi commented on Sep 22, 2019

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions