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

Rpc Server: WitnessRules potential DDOS #2950

Closed
cschuchardt88 opened this issue Sep 12, 2023 · 6 comments · Fixed by neo-project/neo-modules#827 or #2951
Closed

Rpc Server: WitnessRules potential DDOS #2950

cschuchardt88 opened this issue Sep 12, 2023 · 6 comments · Fixed by neo-project/neo-modules#827 or #2951
Assignees
Labels
Discussion Initial issue state - proposed but not yet accepted

Comments

@cschuchardt88
Copy link
Member

Summary or problem description
There is a potential problem with rpc server with signer rules. The neo function see below doesn't pass the max depth for json object. This will allow someone to DDOS all Rpc nodes. I will not post the way to do this here.

Condition = WitnessCondition.FromJson((JObject)json["condition"])

https://github.com/neo-project/neo-modules/blob/09c2879958a916e0867fc78c64a04edfabe6935f/src/RpcServer/RpcServer.SmartContract.cs#L166-L182

Do you have any solution you want to propose?
Put a max depth

Where in the software does this update applies to?

  • RPC (HTTP)
@cschuchardt88 cschuchardt88 added the Discussion Initial issue state - proposed but not yet accepted label Sep 12, 2023
@roman-khimov
Copy link
Contributor

Please check https://github.com/neo-project/neo/security and https://neo.org/bounty for proper reporting of any security issues.

@shargon
Copy link
Member

shargon commented Sep 13, 2023

https://learn.microsoft.com/en-us/dotnet/api/microsoft.aspnetcore.server.kestrel.core.kestrelserverlimits.maxrequestbodysize?view=aspnetcore-7.0

By default, Kestrel has a maximum of 30MB for body length and we already have an authentication mechanism; Btw, we can lower this limit.

@cschuchardt88
Copy link
Member Author

cschuchardt88 commented Sep 13, 2023

But what about Akka? This can affect that as well and ISerializable. But that has limits right? On how big the packet can be? 1MB?

@shargon What is the max allow Conditions for an transaction? If there isn't one I think there should be one.

@shargon
Copy link
Member

shargon commented Sep 15, 2023

The neo function see below doesn't pass the max depth for json object

Do you think that's not enough with

private const int MaxSubitems = 16;
internal const int MaxNestingDepth = 2;
?

@cschuchardt88
Copy link
Member Author

cschuchardt88 commented Sep 16, 2023

Yes.

but if it isn't than they can send another transaction to make up for it?

@cschuchardt88
Copy link
Member Author

cschuchardt88 commented Nov 16, 2023

@roman-khimov Looking at the code again no. Cause it deals with this like code:

if (ReflectionCache<WitnessConditionType>.CreateInstance(type) is not WitnessCondition condition)

You keep creating it, you get it? Idk what else could be using this, but maxdepth is added but here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Discussion Initial issue state - proposed but not yet accepted
Projects
None yet
3 participants