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

SECURITY: Fix #5565 by htmlEncoding titles in issues and milestones #5570

Merged
merged 2 commits into from Dec 21, 2018

Conversation

7 participants
@zeripath
Copy link
Contributor

zeripath commented Dec 20, 2018

There are likely problems remaining with the way that initCommentForm
is creating its elements. I suspect that a malformed avatar url could
be used maliciously.

Fixes the immediate issue in #5565

Immediate fix to htmlEncode user added text
There are likely problems remaining with the way that initCommentForm
is creating its elements. I suspect that a malformed avatar url could
be used maliciously.
@zeripath

This comment has been minimized.

Copy link
Contributor

zeripath commented Dec 20, 2018

Still to look at:

  1. selectItem creates its own elements without necessarily escaping the attributes - in particular it places urls into hrefs without escaping. I'm uncertain whether this is exploitable but the avatar url is the most likely part to be so, if it is possible to exploit.
  2. Repository Descriptions and Milestone descriptions are not being escaped - fortunately <script> elements are being filtered out.
  3. There are probably other elements in the index.js that need some attention but these are unlikely to be exploitable.

@bkcsoft bkcsoft added the lgtm/need 2 label Dec 20, 2018

@zeripath zeripath changed the title Immediate fix to htmlEncode user added text SECURITY: Fix #5565 by htmlEncoding titles in issues and milestones Dec 20, 2018

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Dec 20, 2018

Codecov Report

Merging #5570 into master will decrease coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5570      +/-   ##
==========================================
- Coverage   37.56%   37.55%   -0.02%     
==========================================
  Files         321      321              
  Lines       47206    47206              
==========================================
- Hits        17732    17726       -6     
  Misses      26933    26933              
- Partials     2541     2547       +6
Impacted Files Coverage Δ
modules/process/manager.go 76.81% <0%> (-4.35%) ⬇️
models/repo_indexer.go 44.49% <0%> (-3.82%) ⬇️
routers/repo/view.go 46.01% <0%> (+1.22%) ⬆️
models/unit.go 14.28% <0%> (+14.28%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4a02a78...30a6059. Read the comment docs.

@Bwko

Bwko approved these changes Dec 20, 2018

@bkcsoft bkcsoft added lgtm/need 1 and removed lgtm/need 2 labels Dec 20, 2018

@zeripath

This comment has been minimized.

Copy link
Contributor

zeripath commented Dec 20, 2018

OK regarding point 2. It appears that this is a deliberate decision on behalf of the team. So I won't change this.

Calling @lunny @lafriks - Re: Repository and Markup descriptions being a basic form of html. Is this intentional? It's not obvious from the input fields that this is intentional and that the markup is a shortened form of HTML.

We should probably partially sanitise on save to these fields - otherwise when people query the API they'll get the unsanitised descriptions and will have to sanitise likely getting it wrong.

@lunny

This comment has been minimized.

Copy link
Member

lunny commented Dec 20, 2018

@zeripath why not change server-side to add escape on these two form value?

@zeripath

This comment has been minimized.

Copy link
Contributor

zeripath commented Dec 20, 2018

@lunny I'm not sure I understand. At the moment the Repository and Milestone descriptions are both relatively rich-text markup using a (unspecified) subset of html which is only sanitised on display. There are a few questions:

  • Is it intended that these are meant to be rich text?
  • If so, there's a strange subtlety - links have to be simple text http://foo.com, anchors are completely removed.
  • We should probably specify the type of markup somewhere - it's not clear what's allowed and not.
  • We should also probably sanitise on save so that the user sees the that certain things aren't allowed and so that downstream users aren't caught out by requesting unsanitised data.

If they weren't intended to be rich-text, excellent! We can simply properly escape.

They're both secure as it stands right now though - just potentially unsafe for downstream API users and slightly surprising as a user.

@zeripath

This comment has been minimized.

Copy link
Contributor

zeripath commented Dec 20, 2018

So the rest of index.js appears to be ok.

@techknowlogick techknowlogick added this to the 1.7.0 milestone Dec 20, 2018

@techknowlogick

This comment has been minimized.

Copy link
Member

techknowlogick commented Dec 20, 2018

I agree that we need to look into things more, however I think this is a good interim measure to protect users while we discuss long-term solution. @lunny I'll let you decide if this is an ok approach.

@bkcsoft bkcsoft added lgtm/done and removed lgtm/need 1 labels Dec 21, 2018

@lunny

This comment has been minimized.

Copy link
Member

lunny commented Dec 21, 2018

@zeripath I agree with that it's not clear on Repository and Milestone descriptions supporting rich text. @techknowlogick let's go ahead this PR and make a new issue to discuss that more. Thanks for your work!

@techknowlogick techknowlogick merged commit 330bf8d into go-gitea:master Dec 21, 2018

2 checks passed

approvals/lgtm this commit looks good
continuous-integration/drone/pr the build was successful
Details
@techknowlogick

This comment has been minimized.

Copy link
Member

techknowlogick commented Dec 21, 2018

Created backport at #5575

@zeripath zeripath deleted the zeripath:issue-5565-fix-xss-issues-and-milestones branch Dec 22, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment