Fixes #20128 (Not really fixing it but workaround)
This PR is an alternative to #20393
ACK 578315a
ACK 578315a
I'm not entirely sure what "validating or securely using RPC calls" is referring to. What do you mean by "validating RPC calls" here? And how would one "insecurely" (opposite of securely) use a RPC in this context?
Do you mean it is advised not to reimplement RPCs when building a wallet and to use the ones in Core?
Also a nit: "For example," (what it was originally) is better than "Example"
I'm not entirely sure what "validating or securely using RPC calls" is referring to. What do you mean by "validating RPC calls" here?
Validating "string" input as it is mentioned in "secure string handling" section
And how would one "insecurely" (opposite of securely) use a RPC in this context?
I can share POC if others are okay with it being shared here
Do you mean it is advised not to reimplement RPCs when building a wallet and to use the ones in Core?
No. I just mean implement them in a way that attacker can't exploit your website by using path traversal.
Validating "string" input as it is mentioned in "secure string handling" section
So you are not suggesting anything in addition to what is already in the "Secure string handling" section. Validating what is already in the "Secure string handling" section.
No. I just mean implement them in a way that attacker can't exploit your website by using path traversal.
A suggestion for changes to Bitcoin Core RPCs shouldn't go into docs. They should be discussed in an Issue/PR on their merits which presumably is what is happening in #20128. There appears to be confusion though on the exact change(s) you are suggesting there.
I can share POC if others are okay with it being shared here
I'm not sure this is the best place. Feel free to post on IRC and if it has merits I am sure someone will discuss it with you privately.
As far as I can understand @prayank23 you want to impose additional restrictions on the characters allowed in the wallet name which is being discussed in this PR #20393. That is open and currently in draft status. That appears to have some merit and people are engaging with it.
I don't see the motivation for changing the docs at this stage. If a PR based on #20393 is merged then maybe additional guidance can be considered for the docs (for whatever restrictions are not merged). But until then this particular doc PR is a Concept NACK from me.
So you are not suggesting anything in addition to what is already in the "Secure string handling" section. Validating what is already in the "Secure string handling" section.
I have added one example that I found while testing things related to #20080 (comment)
A suggestion for changes to Bitcoin Core RPCs shouldn't go into docs
This is not a change to RPC
They should be discussed in an Issue/PR on their merits which presumably is what is happening in #20128. There appears to be confusion though on the exact change(s) you are suggesting there.
All the confusion and questions have been answered already including the earlier PR. But the solution suggested in earlier is not the best thing to do because it can break few things in lot of projects unnecessarily. And it can be managed better by just mentioning in docs.
As far as I can understand @prayank23 you want to impose additional restrictions on the characters allowed in the wallet name which is being discussed in this PR #20393. That is open and currently in draft status. That appears to have some merit and people are engaging with it.
Those restrictions was a suggested solution which isn't the best approach to solve this problem IMO. People who are engaging never really tested it so they might find lot of issues with it once they test it on different platforms and projects.
I don't see the motivation for changing the docs at this stage. If a PR based on #20393 is merged then maybe additional guidance can be considered for the docs (for whatever restrictions are not merged). But until then this particular doc PR is a Concept NACK from me.
Thanks for your review and contribution
So you will be closing #20393? You don't think there should be any additional restrictions on the wallet name? Please add a comment to that PR and close it if that is the case.
Instead you want in this doc PR to warn application developers that the Core RPC does not impose these restrictions on the wallet name and hence application developers should be careful to impose application level checks on the wallet names that their users use? Is that correct?
So you will be closing #20393? You don't think there should be any additional restrictions on the wallet name? Please add a comment to that PR and close it if that is the case.
Yes if this gets merged other will be closed. If anyone interested to find a better solution in future can create new PR.
Instead you want in this doc PR to warn application developers that the Core RPC does not impose these restrictions on the wallet name and hence application developers should be careful to impose application level checks on the wallet names that their users use? Is that correct?
Yes. Correct.
89 | @@ -90,11 +90,14 @@ RPC interface will be abused. 90 | although it does usually provide serialized data using a hex 91 | representation of the bytes. If you use RPC data in your programs or 92 | provide its data to other programs, you must ensure any problem 93 | - strings are properly escaped. For example, multiple websites have 94 | + strings are properly escaped. Example: multiple websites have
My alternative suggestion would be:
If you use RPC data in your programs or provide its data to other programs, you must ensure any problem strings are properly escaped. For example, the createwallet RPC accepts arguments such as wallet_name which is a string and could be used for a path traversal attack without application level checks. Multiple websites have been manipulated because they displayed decoded hex strings that included HTML <script> tags. For this reason, and others, it is recommended to display all serialized data in hex form only.
Sorry to be a pain but you'll need to squash your commits :)
ACK from me otherwise
Add information about possible path traversal attack with example
ACK 7117d7503f39f06b74c84777ec4db5d456a8086f
ACK