Skip to content

Conversation

amadeuszpawlik
Copy link
Contributor

@amadeuszpawlik amadeuszpawlik commented Dec 5, 2021

In connection to #22087, it has been pointed out that addpeeraddress needs to get its port-value sanitized.

@ghost
Copy link

ghost commented Dec 7, 2021

Or should I rather validate port directly in addpeeraddress() and just ignore the -0 case?

I think it can be ignored. Tried using -0 in few other arguments for other RPC and it is considered as 0. I could not find a way to read peers.dat however this observation is based on usage of negative zero in other parameters.

Based on reading few posts like https://softwareengineering.stackexchange.com/questions/280648/why-is-negative-zero-important it could be an issue in some cases but not here I guess.I have no opinion on this because I couldn't do anything interesting with -0 in addpeeraddress

@amadeuszpawlik I tried using port -0 in addnode and it adds negative zero in the port, I am assuming similar thing would happen with addpeeraddress so:

Concept ACK for this PR and maybe we need to make changes for few other RPC

@amadeuszpawlik
Copy link
Contributor Author

amadeuszpawlik commented Dec 12, 2021

@prayank23 good find with addnode (although in that particular command the port is specified in an address string, so this wouldn't fix it). It looks like -0 is not the only case that addnode accepts.
The modded SplitHostPort I propose in #22087 would definitely be nice here for port validation.
I'll go trough other RPCs and add validation for them too.

My question still stands, is modifying univalue a valid approach for commands that currently use int for port value, or do I ignore the -0 case and simply validate directly in net.cpp?

@amadeuszpawlik amadeuszpawlik marked this pull request as ready for review February 21, 2022 10:08
@amadeuszpawlik amadeuszpawlik marked this pull request as draft February 21, 2022 10:09
@DrahtBot
Copy link
Contributor

DrahtBot commented May 11, 2022

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

@amadeuszpawlik amadeuszpawlik marked this pull request as ready for review May 13, 2022 20:33
@amadeuszpawlik amadeuszpawlik marked this pull request as draft May 13, 2022 20:42
- Ensures port sanitization in `addpeeraddress()`
- Adds test to check for invalid port values
Copy link
Member

@fanquake fanquake left a comment

Choose a reason for hiding this comment

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

ACK ada8358

@fanquake fanquake merged commit d5d40d5 into bitcoin:master May 17, 2022
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request May 28, 2022
ada8358 Sanitize port in `addpeeraddress()` (amadeuszpawlik)

Pull request description:

  In connection to bitcoin#22087, it has been [pointed out](bitcoin#22087 (review)) that `addpeeraddress` needs to get its port-value sanitized.

ACKs for top commit:
  fanquake:
    ACK ada8358

Tree-SHA512: 48771cd4f6940aa7840fa23488565c09dea86bd5ec5a5a1fc0374afb4857aebcd2a1f51e2d4cb7348460e0ad9793dc5d2962df457084ed2b8d8142cae650003f
@bitcoin bitcoin locked and limited conversation to collaborators May 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants