Validate port-options #22087

amadeuszpawlik · 2021-05-27T14:08:27Z

Validate port-options, so that invalid values are rejected early in the startup.
Ports are uint16_ts, which effectively limits a port's value to <=65535. As discussed in #24116 and #24344, port "0" is considered invalid too.
Proposed in #21893 (comment)

The SplitHostPort(std::string in, uint16_t& portOut, std::string& hostOut) now returns a bool that indicates whether the port value was set and within the allowed range. This is an improvement that can be used not only for port validation of options at startup, but also in rpc calls, etc,

amadeuszpawlik · 2021-05-27T14:14:57Z

Should this be done with -zmqpub... options as well?

jonatack · 2021-05-27T14:18:14Z

I'd suggest searching for and looking at a previous pull request proposal to do this. Consider also unit testing and Doxygen docs.

jonatack · 2021-05-27T14:18:53Z

Consider also the various places in the codebase where users can input port numbers.

DrahtBot · 2021-05-27T15:07:14Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#25136 (Torcontrol opt check by amadeuszpawlik)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

amadeuszpawlik · 2021-05-29T09:43:15Z

Added doxygen descriptions
Added/updated test for SplitHostPort
@jonatack could you clarify

Consider also the various places in the codebase where users can input port numbers.

Have I missed something, like a config?

klementtan

You might wanna add the validation at addpeeraddress too. Currently, the port number will overflow if it is invalid.

amadeuszpawlik · 2021-06-03T16:09:09Z

Thanks for your input, @klementtan !
I addressed your comment in a1c591453fbd3f28772a230720675f4e9ff74978

klementtan

Thanks for updating it!

I think you might have missed out the scenario where the user enters -1 as the port. On a1c591453fbd3f28772a230720675f4e9ff74978

$ ./src/bitcoin-cli -signet addpeeraddress "1.1.1.3" -1
{
  "success": true
}

You might wanna refactor the logic in strencodings.cpp so that the port validation code could be reused in other places(ie addpeeraddress)

src/util/strencodings.cpp

amadeuszpawlik · 2021-06-04T13:08:56Z

@klementtan

think you might have missed out the scenario where the user enters -1 as the port.

You are absolutely right, fixed that. I also added a couple negative port numbers in the tests, thanks.

laanwj · 2021-08-02T12:08:06Z

Concept ACK. I would personally call this "validate" (as in input validation), not "santize". In the source base we use "sanitize" only in the context of removing invalid characters from strings in logging.

src/util/strencodings.h

amadeuszpawlik · 2021-08-27T13:58:12Z

@laanwj

Concept ACK. I would personally call this "validate" (as in input validation), not "santize". In the source base we use "sanitize" only in the context of removing invalid characters from strings in logging.

I have changed the wording, thanks 👍

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

src/init.cpp

ryanofsky

Reviewed 1dae86bfd2259e4d9a9f1cd292a10cc17f626876, and it looks almost ok, but I am concerned different SplitHostPort hostOut values when port is invalid could potentially be bad for callers. Or even if it doesn't break anything, it seems better just to add a additional return value not change hostOut values in this PR.

It would also be good to note the change of behavior in the PR description or release notes that -port and -rpcport values that previously worked and were considered valid can now result in errors. For example port values with leading whitespace or trailing non-digit characters.

I do think code in second commit is a little awkward because it is parsing and validating address and port options in different place where they are used. But probably this is fine, since in the future this will probably be cleaned up by introducing options structs and moving new code here to a function like ApplyArgsManOptions that does parsing and error-checking all in one place.

src/util/strencodings.cpp

src/test/netbase_tests.cpp

src/init.cpp

fanquake · 2022-10-02T15:28:26Z

@amadeuszpawlik any chance you're still around / interested in responding to the latest review feedback?

amadeuszpawlik · 2022-10-03T08:00:39Z

@amadeuszpawlik any chance you're still around / interested in responding to the latest review feedback?

@fanquake Yes, definitely. Will have a go at it shortly 👍

src/util/strencodings.cpp

Forward the validation of the port from `ParseUInt16(...)`. Consider port 0 as invalid. Add suitable test for the `SplitHostPort` function. Add doxygen description to the `SplitHostPort` function.

Check `port` options for invalid values (ports are parsed as uint16, so in practice values >65535 are invalid; port 0 is undefined and therefore considered invalid too). This allows for an early rejection of faulty values and an supplying an informative message to the user. Splits tests in `feature_proxy.py` to cover both invalid `hostname` and `port` values. Adds a release-note as previously valid `-port` and `-rpcport` values can now result in errors.

amadeuszpawlik · 2022-10-05T17:33:17Z

Thanks @ryanofsky for taking your time. I have now resolved all your comments and added a release-note.

ryanofsky

Code review ACK 0452678. Just suggested changes since last review: reverting some SplitHostPort changes, adding release notes, avoiding 'GetArgs[0]` problem.

luke-jr · 2022-10-06T00:37:44Z

utACK 0452678

luke-jr · 2022-10-06T00:39:06Z

(In the future, though, please don't rebase every time you make changes - only when needed)

fanquake · 2022-10-11T07:23:37Z

@ryanofsky just fyi. You've re-ACK the previous commit hash in your latest comment

ryanofsky · 2022-10-11T16:23:46Z

@ryanofsky just fyi. You've re-ACK the previous commit hash in your latest comment

Thank you, fixed now

amadeuszpawlik changed the title ~~Sanitize ports~~ Sanitize port numbers May 27, 2021

DrahtBot added the Utils/log/libs label May 27, 2021

amadeuszpawlik changed the title ~~Sanitize port numbers~~ WIP Sanitize port numbers May 27, 2021

DrahtBot mentioned this pull request May 27, 2021

util: Replace use of locale dependent atoi(…) with locale-independent std::from_chars(…) (C++17) #20452

Merged

amadeuszpawlik force-pushed the sanitize_ports branch 3 times, most recently from 9850393 to 2b6eed6 Compare May 29, 2021 09:25

amadeuszpawlik changed the title ~~WIP Sanitize port numbers~~ Sanitize port numbers May 31, 2021

amadeuszpawlik marked this pull request as ready for review May 31, 2021 05:07

DrahtBot mentioned this pull request May 31, 2021

p2p: Ignore ports on I2P addresses #21514

Closed

klementtan reviewed Jun 3, 2021

View reviewed changes

src/util/strencodings.cpp Outdated Show resolved Hide resolved

amadeuszpawlik force-pushed the sanitize_ports branch from a1c5914 to 96942e8 Compare June 4, 2021 13:07

laanwj reviewed Aug 2, 2021

View reviewed changes

src/util/strencodings.h Outdated Show resolved Hide resolved

amadeuszpawlik force-pushed the sanitize_ports branch from 96942e8 to 3309b68 Compare August 27, 2021 13:55

amadeuszpawlik changed the title ~~Sanitize port numbers~~ Validate port numbers Aug 27, 2021

DrahtBot added the Needs rebase label Sep 21, 2021

amadeuszpawlik force-pushed the sanitize_ports branch from 95bc9bd to effb74f Compare December 1, 2021 19:41

luke-jr reviewed Jun 12, 2022

View reviewed changes

src/init.cpp Outdated Show resolved Hide resolved

amadeuszpawlik force-pushed the sanitize_ports branch from ce4652a to 1dae86b Compare June 13, 2022 14:57

ghost mentioned this pull request Jun 30, 2022

Rephrase error message for invalid value of -peertimeout #25506

Merged

ryanofsky reviewed Sep 27, 2022

View reviewed changes

src/util/strencodings.cpp Outdated Show resolved Hide resolved

src/util/strencodings.cpp Outdated Show resolved Hide resolved

src/test/netbase_tests.cpp Outdated Show resolved Hide resolved

src/init.cpp Outdated Show resolved Hide resolved

amadeuszpawlik force-pushed the sanitize_ports branch 4 times, most recently from 72870e1 to 7a7eb90 Compare October 3, 2022 18:41

luke-jr reviewed Oct 3, 2022

View reviewed changes

src/util/strencodings.cpp Outdated Show resolved Hide resolved

amadeuszpawlik force-pushed the sanitize_ports branch 2 times, most recently from 9c2031d to de27526 Compare October 5, 2022 17:05

amadeuszpawlik added 2 commits October 5, 2022 19:24

Validate port value in SplitHostPort

f8387c4

Forward the validation of the port from `ParseUInt16(...)`. Consider port 0 as invalid. Add suitable test for the `SplitHostPort` function. Add doxygen description to the `SplitHostPort` function.

amadeuszpawlik force-pushed the sanitize_ports branch from de27526 to 0452678 Compare October 5, 2022 17:31

ryanofsky approved these changes Oct 5, 2022

View reviewed changes

fanquake merged commit 5fc3939 into bitcoin:master Oct 12, 2022

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Oct 23, 2022

Merge bitcoin#22087: Validate port-options

21024d1

JayBitron mentioned this pull request Jul 2, 2023

exclude ipc scheme from port check #28020

Closed

guggero mentioned this pull request Dec 18, 2023

GitHub: use bitcoind v26.0 for CI lightningnetwork/lnd#8273

Merged

fanquake mentioned this pull request Apr 4, 2024

Invalid port specified in -zmqpubrawtx when using IPC #29802

Closed

1 task

guggero mentioned this pull request Apr 19, 2024

release: create v0.17.5 release branch lightningnetwork/lnd#8664

Merged

bitcoin locked and limited conversation to collaborators Jul 1, 2024

Validate port-options #22087

Validate port-options #22087

Uh oh!

Conversation

amadeuszpawlik commented May 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amadeuszpawlik commented May 27, 2021

Uh oh!

jonatack commented May 27, 2021

Uh oh!

jonatack commented May 27, 2021

Uh oh!

DrahtBot commented May 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

amadeuszpawlik commented May 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

klementtan left a comment

Choose a reason for hiding this comment

Uh oh!

amadeuszpawlik commented Jun 3, 2021

Uh oh!

klementtan left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

amadeuszpawlik commented Jun 4, 2021

Uh oh!

laanwj commented Aug 2, 2021

Uh oh!

Uh oh!

amadeuszpawlik commented Aug 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

ryanofsky left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fanquake commented Oct 2, 2022

Uh oh!

amadeuszpawlik commented Oct 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

amadeuszpawlik commented Oct 5, 2022

Uh oh!

ryanofsky left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

luke-jr commented Oct 6, 2022

Uh oh!

luke-jr commented Oct 6, 2022

Uh oh!

fanquake commented Oct 11, 2022

Uh oh!

ryanofsky commented Oct 11, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

amadeuszpawlik commented May 27, 2021 •

edited

Loading

DrahtBot commented May 27, 2021 •

edited

Loading

amadeuszpawlik commented May 29, 2021 •

edited

Loading

klementtan left a comment •

edited

Loading

amadeuszpawlik commented Aug 27, 2021 •

edited

Loading

ryanofsky left a comment •

edited

Loading

amadeuszpawlik commented Oct 3, 2022 •

edited

Loading

ryanofsky left a comment •

edited

Loading