Skip to content

Conversation

@jonasfranz
Copy link
Member

Fix #4307 by checking if URL is external before redirecting.

Affected:

  • 2FA
  • U2F
  • Normal login

Signed-off-by: Jonas Franz <info@jonasfranz.software>
Signed-off-by: Jonas Franz <info@jonasfranz.software>
Signed-off-by: Jonas Franz <info@jonasfranz.software>
@codecov-io
Copy link

codecov-io commented Jun 25, 2018

Codecov Report

Merging #4312 into master will increase coverage by 0.01%.
The diff coverage is 60%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #4312      +/-   ##
=========================================
+ Coverage   20.09%   20.1%   +0.01%     
=========================================
  Files         153     153              
  Lines       30696   30705       +9     
=========================================
+ Hits         6168    6174       +6     
- Misses      23586   23588       +2     
- Partials      942     943       +1
Impacted Files Coverage Δ
routers/user/auth.go 0% <0%> (ø) ⬆️
modules/util/util.go 36% <66.66%> (+6.73%) ⬆️

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 b8c2420...3913603. Read the comment docs.

@bkcsoft bkcsoft added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jun 25, 2018
newTest(true,
"http://example.com"),
newTest(false,
"a/"),
Copy link
Member

Choose a reason for hiding this comment

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

Why is this false?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because it is not external

"a/"),
newTest(false,
"https://try.gitea.io/test?param=false"),
newTest(false,
Copy link
Member

Choose a reason for hiding this comment

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

Same here

Copy link
Member Author

Choose a reason for hiding this comment

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

Because it is not external

"test?param=false"),
newTest(false,
"//try.gitea.io/test?param=false"),
newTest(false,
Copy link
Member

Choose a reason for hiding this comment

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

And here

Copy link
Member Author

Choose a reason for hiding this comment

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

Because it is not external

@techknowlogick techknowlogick added the topic/security Something leaks user information or is otherwise vulnerable. Should be fixed! label Jun 25, 2018
@techknowlogick techknowlogick added this to the 1.5.0 milestone Jun 25, 2018
@bkcsoft bkcsoft added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Jun 25, 2018
@lunny
Copy link
Member

lunny commented Jun 26, 2018

LGTM

@bkcsoft bkcsoft added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Jun 26, 2018
Copy link
Member

@strk strk left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

@lunny lunny merged commit 801843b into go-gitea:master Jun 26, 2018
@techknowlogick
Copy link
Member

@JonasFranzDEV could you backport this to 1.4 as well?

@bkcsoft
Copy link
Member

bkcsoft commented Jun 26, 2018

I'm working on a backport to 1.4

bkcsoft pushed a commit that referenced this pull request Jun 26, 2018
@bkcsoft bkcsoft mentioned this pull request Jun 26, 2018
bkcsoft added a commit that referenced this pull request Jun 26, 2018
@jonasfranz jonasfranz deleted the fix-open-redirect branch June 27, 2018 21:03
@sapk sapk mentioned this pull request Jun 28, 2018
7 tasks
if err != nil {
return true
}
if len(parsed.Host) != 0 && strings.Replace(parsed.Host, "www.", "", 1) != strings.Replace(setting.Domain, "www.", "", 1) {
Copy link
Member

@sapk sapk Jun 28, 2018

Choose a reason for hiding this comment

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

@ghost ghost mentioned this pull request Jun 28, 2018
7 tasks
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. topic/security Something leaks user information or is otherwise vulnerable. Should be fixed!

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Open redirect vulnerability on 2FA

8 participants