Skip to content
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

tests and fixes for issue #69 #71

Merged
merged 2 commits into from Mar 4, 2021
Merged

Conversation

isedev
Copy link

@isedev isedev commented Mar 3, 2021

2As suggested in PR#70, a new pull request which captures just the tests proving issue #69, with some refinements:

  • header overflow (i.e. reads more than the bufio.Reader default buffer size of 4096).
  • "slow loris" type vulnerability, where the header read blocks waiting for terminator.

The second issue above can be used for DoS by consuming server process sockets.

Will then submit fixes - further testing shows that pull request #70 does not solve the problem properly.

Fixes #69

@isedev isedev closed this Mar 3, 2021
@isedev isedev deleted the feature/issue-69-tests branch March 3, 2021 02:44
@isedev isedev restored the feature/issue-69-tests branch March 3, 2021 02:46
@isedev isedev reopened this Mar 3, 2021
@coveralls
Copy link

coveralls commented Mar 3, 2021

Coverage Status

Coverage decreased (-94.2%) to 0.0% when pulling a368adc on isedev:feature/issue-69-tests into c4bcea2 on pires:main.

@isedev
Copy link
Author

isedev commented Mar 3, 2021

Added the new fix. Let me know what you think.

@isedev isedev changed the title add tests proving issue #69 tests and fixes for issue #69 Mar 3, 2021
Copy link
Owner

@pires pires left a comment

Choose a reason for hiding this comment

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

The code mostly LGTM. Other than two comments I kindly request you tidy up the commit log.

I'd also like to compensate you for this contribution as it seems a serious security issue. Can you please reach out over Twitter (@therealpires) or email pjpires at googlemail?

Thanks a ton!

v1.go Outdated Show resolved Hide resolved
v1.go Outdated Show resolved Hide resolved
@pires pires self-assigned this Mar 3, 2021
@pires pires added the bug label Mar 3, 2021
@pires pires added this to the 0.5 milestone Mar 3, 2021
@pires
Copy link
Owner

pires commented Mar 3, 2021

cc @emersion @mschneider82 @TimWolla @jefferai (I wish I remembered everyone's name/handle). Someone at Snyk is also willing to help getting a CVE for this which will be a first for me. Thanks everybody out there paying attention and willing to help.

@TimWolla
Copy link
Contributor

TimWolla commented Mar 3, 2021

Thanks for the ping! I'm afraid I won't be much of a help here, though. I'm not even a go programmer. My contribution regarding the unique ID TLV was motivated by the fact that I was the person that added this TLV to the PROXYv2 spec (I'm somewhat involved with upstream HAProxy) and I just searched for proxy protocol libraries to make sure they are up to date.

v1.go Outdated Show resolved Hide resolved
@jefferai
Copy link

jefferai commented Mar 3, 2021

@pires Thanks for pinging me! This generally looks good, left a couple of comments.

@isedev
Copy link
Author

isedev commented Mar 3, 2021

The code mostly LGTM. Other than two comments I kindly request you tidy up the commit log.

Any suggestions how I tidy up the commit log? I agree needs to be done, just no idea how to without new pull request. If that's what it takes, more than happy to.

@isedev isedev requested a review from pires March 3, 2021 19:32
@TimWolla
Copy link
Contributor

TimWolla commented Mar 3, 2021

Any suggestions how I tidy up the commit log?

Use an interactive rebase git rebase -i and then force push to isedev:feature/issue-69-tests.

@isedev
Copy link
Author

isedev commented Mar 3, 2021

Any suggestions how I tidy up the commit log?

Use an interactive rebase git rebase -i and then force push to isedev:feature/issue-69-tests.

Thanks. Learn something new everyday :)

Copy link
Owner

@pires pires left a comment

Choose a reason for hiding this comment

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

LGTM, thanks a tonne!

@pires pires merged commit 7f48261 into pires:main Mar 4, 2021
@mschneider82
Copy link
Contributor

@pires can you tag a new release?

@pires
Copy link
Owner

pires commented Mar 4, 2021

Working on it. Want to get Go 1.16 in and some fixes to tests I spotted while doing so.

@mschneider82
Copy link
Contributor

i do a pull req for traefik, where i used this lib to provide proxy protocol support, but traefik still depends on go 1.15, so requiring 1.16 here would break it. I will stick on last commit for now

@pires
Copy link
Owner

pires commented Mar 4, 2021

There's no such thing as enforcing Go 1.16 requirement. CI compiles and tests for Go 1.14 and Go 1.15, and now want to add 1.16, that's all.

@mschneider82
Copy link
Contributor

Ok fine. The traefik maintainers requested a new version tag

@pires
Copy link
Owner

pires commented Mar 5, 2021

OK the tests fixing will have to wait as I need more time to find a reasonable strategy while also refactoring to reduce lots of duplicated code.

@pires
Copy link
Owner

pires commented Mar 5, 2021

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

parseVersion1() is not secure
6 participants