Sanitize port in addpeeraddress() #23679

1. 
   amadeuszpawlik commented at 8:55 pm on December 5, 2021: contributor

   In connection to #22087, it has been pointed out that addpeeraddress needs to get its port-value sanitized.
2. DrahtBot added the label RPC/REST/ZMQ on Dec 5, 2021
3. DrahtBot added the label Upstream on Dec 5, 2021
4. DrahtBot added the label Utils/log/libs on Dec 5, 2021
5. 
   ghost commented at 11:10 pm on December 7, 2021: none

   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

6. 
   amadeuszpawlik commented at 2:12 pm on December 12, 2021: contributor

   @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?

7. amadeuszpawlik marked this as ready for review on Feb 21, 2022
8. amadeuszpawlik marked this as a draft on Feb 21, 2022
9. 
   DrahtBot commented at 3:27 pm on May 11, 2022: member

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

   Conflicts

   No conflicts as of last run.

10. DrahtBot added the label Needs rebase on May 13, 2022
11. in src/rpc/net.cpp:946 in 0f72ddfbe5 outdated

    942@@ -943,7 +943,7 @@ static RPCHelpMan addpeeraddress()
    943     }
    944 
    945     const std::string& addr_string{request.params[0].get_str()};
    946-    const uint16_t port{static_cast<uint16_t>(request.params[1].get_int())};
    947+    const uint16_t& port{request.params[1].get_uint16()};
    

    ---

    ---

    

    MarcoFalke commented at 10:09 am on May 13, 2022:

    0    const auto port{request.params[1].getInt<uint16_t>()};
    

    on master this is now trivial to implement in a one-line diff

12. amadeuszpawlik force-pushed on May 13, 2022
13. amadeuszpawlik marked this as ready for review on May 13, 2022
14. amadeuszpawlik marked this as a draft on May 13, 2022
15. DrahtBot removed the label Needs rebase on May 13, 2022
16. amadeuszpawlik force-pushed on May 14, 2022
17. Sanitize port in `addpeeraddress()`

    - Ensures port sanitization in `addpeeraddress()`
    - Adds test to check for invalid port values

    ada8358ef5
18. amadeuszpawlik force-pushed on May 14, 2022
19. amadeuszpawlik marked this as ready for review on May 14, 2022
20. MarcoFalke removed the label RPC/REST/ZMQ on May 14, 2022
21. MarcoFalke removed the label Upstream on May 14, 2022
22. MarcoFalke removed the label Utils/log/libs on May 14, 2022
23. DrahtBot added the label RPC/REST/ZMQ on May 14, 2022
24. fanquake approved
25. 
    fanquake commented at 3:38 pm on May 17, 2022: member

    ACK ada8358ef54aaa04c9182afe115d8046c801bdde
26. fanquake merged this on May 17, 2022
27. fanquake closed this on May 17, 2022

    ---

28. sidhujag referenced this in commit ec374e7e02 on May 28, 2022
29. DrahtBot locked this on May 17, 2023

Contributors
amadeuszpawlik ghost DrahtBot MarcoFalke fanquake

Labels
RPC/REST/ZMQ