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

[New] RestServer Plugin #3390

Open
wants to merge 66 commits into
base: master
Choose a base branch
from

Conversation

cschuchardt88
Copy link
Member

@cschuchardt88 cschuchardt88 commented Jul 2, 2024

Description

In this section you will learn about RestServer plugin and how it works.

Checkout the docs in ./docs/RestServer folder.

Dependencies

  • Microsoft.AspNetCore.JsonPatch.dll Required
  • Microsoft.AspNetCore.Mvc.NewtonsoftJson.dll Required
  • Microsoft.AspNetCore.Mvc.Versioning.ApiExplorer.dll Required
  • Microsoft.AspNetCore.Mvc.Versioning.dll Required
  • Microsoft.OpenApi.dll Required
  • Newtonsoft.Json.Bson.dll Required
  • Newtonsoft.Json.dll Required
  • System.ServiceProcess.ServiceController.dll linux maybe
  • Microsoft.AspNetCore.Mvc.Versioning.dll Required
  • Microsoft.AspNetCore.Mvc.Versioning.dll Required
  • Microsoft.AspNetCore.Mvc.Versioning.dll Required
  • Microsoft.OpenApi.dll Swagger (optional)
  • Swashbuckle.AspNetCore.Swagger.dll Swagger (optional)
  • Swashbuckle.AspNetCore.SwaggerGen.dll Swagger (optional)
  • Swashbuckle.AspNetCore.SwaggerUI.dll Swagger (optional)
  • Swashbuckle.AspNetCore.Newtonsoft.dll Swagger (optional)
  • RestServer.xml Swagger UI (optional)

In Docker

RestServer
|   |   |-- Microsoft.AspNetCore.JsonPatch.dll
|   |   |-- Microsoft.AspNetCore.Mvc.NewtonsoftJson.dll
|   |   |-- Microsoft.AspNetCore.Mvc.Versioning.ApiExplorer.dll
|   |   |-- Microsoft.AspNetCore.Mvc.Versioning.dll
|   |   |-- Microsoft.OpenApi.dll
|   |   |-- Neo.ConsoleService.dll
|   |   |-- Newtonsoft.Json.Bson.dll
|   |   |-- Newtonsoft.Json.dll
|   |   |-- RestServer.deps.json
|   |   |-- RestServer.dll
|   |   |-- RestServer.pdb
|   |   |-- RestServer.xml
|   |   |-- Swashbuckle.AspNetCore.Newtonsoft.dll
|   |   |-- Swashbuckle.AspNetCore.Swagger.dll
|   |   |-- Swashbuckle.AspNetCore.SwaggerGen.dll
|   |   |-- Swashbuckle.AspNetCore.SwaggerUI.dll
|   |   |-- System.ServiceProcess.ServiceController.dll
|   |   |-- config.json
|   |   `-- runtimes
|   |       `-- win
|   |           `-- lib
|   |               `-- net6.0
|   |                   `-- System.ServiceProcess.ServiceController.dll

These files go in the same directory as the RestServer.dll. In neo-cli
plugins/RestServer/ folder.

Response Headers

Name Value(s) Description
server neo-cli/3.6.0 RestServer/3.6.0 neo-cli and RestServer version.

JSON Serializer

RestServer uses custom Newtonsoft Json Converters to serialize controller action
responses and route parameters.

One Way Binding - Write only.

  • Neo.SmartContract.ContractState
  • Neo.SmartContract.NefFile
  • Neo.SmartContract.MethodToken
  • Neo.SmartContract.Native.TrimmedBlock
  • Neo.SmartContract.Manifest.ContractAbi
  • Neo.SmartContract.Manifest.ContractGroup
  • Neo.SmartContract.Manifest.ContractManifest
  • Neo.SmartContract.Manifest.ContractPermission
  • Neo.SmartContract.Manifest.ContractPermissionDescriptor
  • Neo.Network.P2P.Payloads.Block
  • Neo.Network.P2P.Payloads.Header
  • Neo.Network.P2P.Payloads.Signer
  • Neo.Network.P2P.Payloads.TransactionAttribute
  • Neo.Network.P2P.Payloads.Transaction
  • Neo.Network.P2P.Payloads.Witness

Two Way Binding - Read & Write

  • System.Guid
  • System.ReadOnlyMemory<T>
  • Neo.BigDecimal
  • Neo.UInt160
  • Neo.UInt256
  • Neo.Cryptography.ECC.ECPoint
  • Neo.VM.Types.Array
  • Neo.VM.Types.Boolean
  • Neo.VM.Types.Buffer
  • Neo.VM.Types.ByteString
  • Neo.VM.Types.Integer
  • Neo.VM.Types.InteropInterface
  • Neo.VM.Types.Null
  • Neo.VM.Types.Map
  • Neo.VM.Types.Pointer
  • Neo.VM.Types.StackItem
  • Neo.VM.Types.Struct

Remote Endpoints

Parametes {hash} can be any Neo N3 address or scripthash; {address} can be any Neo N3 address only; {number} and {index} can be any uint32.

Parameter Examples

  • {hash} - 0xef4073a0f2b305a38ec4050e4d3d28bc40ea63f5 or NiHURyS83nX2mpxtA7xq84cGxVbHojj5Wc
  • {address} - NiHURyS83nX2mpxtA7xq84cGxVbHojj5Wc
  • {number} - 1
  • {index} - 2500000

Paths

  • Utils
    • [GET] /api/v1/utils/{hash}/address
    • [GET] /api/v1/utils/{address}/scripthash
    • [GET] /api/v1/utils/{hash}/{address}/validate
  • Node
    • [GET] /api/v1/node/peers
    • [GET] /api/v1/node/plugins
    • [GET] /api/v1/node/settings
  • Ledger
    • [GET] /api/v1/ledger/neo/accounts
    • [GET] /api/v1/ledger/gas/accounts
    • [GET] /api/v1/ledger/blocks?page={number}&size={number}
    • [GET] /api/v1/ledger/blocks/height
    • [GET] /api/v1/ledger/blocks/{index}
    • [GET] /api/v1/ledger/blocks/{index}/header
    • [GET] /api/v1/ledger/blocks/{index}/witness
    • [GET] /api/v1/ledger/blocks/{index}/transactions?page={number}&size={number}
    • [GET] /api/v1/ledger/transactions/{hash}
    • [GET] /api/v1/ledger/transactions/{hash}/witnesses
    • [GET] /api/v1/ledger/transactions/{hash}/signers
    • [GET] /api/v1/ledger/transactions/{hash}/attributes
    • [GET] /api/v1/ledger/memorypool?page={number}&size={number}
    • [GET] /api/v1/ledger/memorypool/verified?page={number}&size={number}
    • [GET] /api/v1/ledger/memorypool/unverified?page={number}&size={number}
    • [GET] /api/v1/ledger/memorypool/count
  • Tokens
    • [GET] /api/v1/tokens/balanceof/{address}
    • NFTs
      • [GET] /api/v1/tokens/nep-11?page={number}&size={number}
      • [GET] /api/v1/tokens/nep-11/count
      • [GET] /api/v1/tokens/nep-11/{hash}/balanceof/{address}
    • NEP-17
      • [GET] /api/v1/tokens/nep-17?page={number}&size={number}
      • [GET] /api/v1/tokens/nep-17/count
      • [GET] /api/v1/tokens/nep-17/{hash}/balanceof/{address}
  • Contracts
    • [GET] /api/v1/contracts?page={number}&size={number}
    • [GET] /api/v1/contracts/count
    • [GET] /api/v1/contracts/{hash}
    • [GET] /api/v1/contracts/{hash}/abi
    • [GET] /api/v1/contracts/{hash}/manifest
    • [GET] /api/v1/contracts/{hash}/nef
    • [GET] /api/v1/contracts/{hash}/storage

image

Type of change

  • Optimization (the change is only an optimization)
  • Style (the change is only a code style for better maintenance or standard purpose)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

By @superboyiii from neo-project/neo-modules#839

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@superboyiii
Copy link
Member

superboyiii commented Jul 8, 2024

All contractstates are missing updatecounter in Contracts collections. Maybe need make result as contracts.ToJson().
6f65ec516137424101eb0bd44811430

throw new QueryParameterNotFoundException(nameof(method));
try
{
var engine = ScriptHelper.InvokeMethod(_neosystem.Settings, _neosystem.StoreView, contracts.Hash, method, contractParameters, out var script);
Copy link
Member

Choose a reason for hiding this comment

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

Need supporting signer account and scope like what invokefunction did, or methods can't pass verifywitness.

Copy link
Member Author

Choose a reason for hiding this comment

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

image

@Jim8y
Copy link
Contributor

Jim8y commented Jul 27, 2024

@cschuchardt88 conflict

@superboyiii
Copy link
Member

superboyiii commented Aug 8, 2024

ff5dbfcd28b2a52b49f4f023314bb84
result[0] of BalanceOf can be type of any.
97eeb99fd9029e21071b57bf1c27162
@cschuchardt88

@shargon
Copy link
Member

shargon commented Feb 10, 2025

Ban method should be by endPoint instead of controller before merge, some methods are prone to denial of service

@cschuchardt88
Copy link
Member Author

Maybe neo functions can be denied service. As for the webserver it won't be acceptable to an attack. However webserver calling neo functions, maybe...

Can you give a working example?

@shargon
Copy link
Member

shargon commented Feb 10, 2025

Can you give a working example?

ShowGasAccounts with one million accounts

@cschuchardt88
Copy link
Member Author

cschuchardt88 commented Feb 10, 2025

Nothing will happen to the server in that case. The page will just not load. The reason I didn't page it is, because I checked testnet and maintest and wasnt that many accounts that actually hold gas or neo. I believe the record gets deleted. I think until we have million records, we shouldn't worry about it.

A DOS happens when a server is manipulating data or killing a thread or process.

Copy link
Member

@shargon shargon left a comment

Choose a reason for hiding this comment

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

It must be secure, and now it is prone to denial of service.

@cschuchardt88
Copy link
Member Author

cschuchardt88 commented Feb 10, 2025

It must be secure, and now it is prone to denial of service.

It doesn't affect the node or the server at all. The request will timeout. If user stops the request this will just cancel the iterator. There is no different if the http server or cli ran the same function.

Timeout Settings

options.Limits.KeepAliveTimeout = TimeSpan.FromSeconds(_settings.KeepAliveTimeout); // default is 120 seconds in 'RestServer.json'
options.Limits.RequestHeadersTimeout = TimeSpan.FromSeconds(15);

I have done my R&D and this is why I added Timeout to the settings. So stuff like this can't proc.

@shargon
Copy link
Member

shargon commented Feb 10, 2025

It must be secure, and now it is prone to denial of service.

It doesn't affect the node or the server at all. The request will timeout. If user stops the request this will just cancel the iterator. There is no different if the http server or cli ran the same function.

Timeout Settings

options.Limits.KeepAliveTimeout = TimeSpan.FromSeconds(_settings.KeepAliveTimeout); // default is 120 seconds in 'RestServer.json'
options.Limits.RequestHeadersTimeout = TimeSpan.FromSeconds(15);

I have done my R&D and this is why I added Timeout to the settings. So stuff like this can't proc.

No, is not, the request can be canceled, but the iteration don't receive the cancelationRequest, so it will iterate until end, and another thread can do the same until denail the service.

@cschuchardt88
Copy link
Member Author

cschuchardt88 commented Feb 11, 2025

No, is not, the request can be canceled, but the iteration don't receive the cancelationRequest, so it will iterate until end, and another thread can do the same until denail the service.

Unless you have working example or evidence, http in dotnet doesn't work like this. It will cancel the thread that is running the iterator. There is no other thread that is running in the background or that was created for the iterator. It runs the thread where the request was executed. I recommend you do a little reading on kestrel about its life cycle. Now we can disable KeepAlive on that request that uses the iterator. So as soon as they disconnect it stops the thread.

Request draining with ASP.NET Core Kestrel web server

  • Kestrel tries to drain the request body. Draining the request body means reading and discarding the data without processing it.
  • Draining has a timeout of five seconds, which isn't configurable.
  • If all of the data specified by the Content-Length or Transfer-Encoding header hasn't been read before the timeout, the connection is closed.

https://learn.microsoft.com/en-us/aspnet/core/fundamentals/servers/kestrel/request-draining?view=aspnetcore-9.0

@dusmart
Copy link

dusmart commented Feb 11, 2025

Can you give a working example?

ShowGasAccounts with one million accounts

Thank you for identifying this issue. This is indeed a significant DoS entry point. A simple request can consume considerable CPU and memory resources, especially given the current number of accounts.

Reference:

public static IEnumerable<(UInt160 Address, BigInteger Balance)> GetAccounts(this GasToken gasToken, DataCache snapshot)
{
if (gasToken is null)
throw new ArgumentNullException(nameof(gasToken));
if (snapshot is null)
throw new ArgumentNullException(nameof(snapshot));
var kb = new KeyBuilder(gasToken.Id, GasToken.Prefix_Account);
var prefixKey = kb.ToArray();
foreach (var (key, value) in snapshot.Find(prefixKey, SeekDirection.Forward))
{
var keyBytes = key.ToArray();
var addressHash = new UInt160(keyBytes.AsSpan(prefixKey.Length));
yield return new(addressHash, value.GetInteroperable<AccountState>().Balance);
}
}
)

@shargon
Copy link
Member

shargon commented Feb 11, 2025

No, is not, the request can be canceled, but the iteration don't receive the cancelationRequest, so it will iterate until end, and another thread can do the same until denail the service.

Unless you have working example or evidence, http in dotnet doesn't work like this. It will cancel the thread that is running the iterator. There is no other thread that is running in the background or that was created for the iterator. It runs the thread where the request was executed. I recommend you do a little reading on kestrel about its life cycle. Now we can disable KeepAlive on that request that uses the iterator. So as soon as they disconnect it stops the thread.

Request draining with ASP.NET Core Kestrel web server

  • Kestrel tries to drain the request body. Draining the request body means reading and discarding the data without processing it.
  • Draining has a timeout of five seconds, which isn't configurable.
  • If all of the data specified by the Content-Length or Transfer-Encoding header hasn't been read before the timeout, the connection is closed.

https://learn.microsoft.com/en-us/aspnet/core/fundamentals/servers/kestrel/request-draining?view=aspnetcore-9.0

Trust in security experts, it will be better for you, this PR contains multiple Denial of Services end points.
I have not asked to delete them, just to secure them, I do not understand why that is a problem for you.

@cschuchardt88
Copy link
Member Author

cschuchardt88 commented Feb 11, 2025

Because your two's claims are built on assumptions. Mine are built on facts. see #3390 (comment)

If this is true then NativeContract.ListContracts is DOSable in RpcServer. Also thats goes for getting blocks with transactions if there are 10k transactions in a block in RpcServer.

@dusmart
Copy link

dusmart commented Feb 11, 2025

If this is true then NativeContract.ListContracts is DOSable in RpcServer. Also thats goes for getting blocks with transactions if there are 10k transactions in a block in RpcServer.

Thank you for pointing it out. Where is it in the RpcServer? The code is introduced in #3614.

@cschuchardt88
Copy link
Member Author

Thank you for pointing it out. Where is it in the RpcServer? The code is introduced in #3614.

foreach (UInt160 account in wallet.GetAccounts().Select(p => p.ScriptHash))

protected internal virtual JToken ListAddress(JArray _params)

@dusmart
Copy link

dusmart commented Feb 11, 2025

foreach (UInt160 account in wallet.GetAccounts().Select(p => p.ScriptHash))

protected internal virtual JToken ListAddress(JArray _params)

That’s not a major concern, as most wallets don’t contain many addresses. Additionally, openwallet is disabled by default in the RPCServer. However, in the current plugin, a simple curl http://127.0.0.1:10339/api/v1/ledger/gas/accounts request returns 7,929,779 characters in my test, which is a significant payload. I believe these two scenarios present different levels of risk. But in theory, you're right about it. It's better to protect it, either.

Because your two's claims are built on assumptions. Mine are built on facts. see #3390 (comment)

I feel disheartened by this response. I invested time and effort into conducting tests to help improve NEO's security, and it’s difficult for me to accept being dismissed this way. I truly appreciate productive discussions and differences in perspective, but let’s keep the focus on the PR itself and avoid making it personal.

Your expertise in the .NET field is undeniable, and I respect that. However, this doesn’t change the core issue—users can indeed place a significant load on a node with a simple request.

That said, I hadn’t initially noticed this problem, thanks to the good design in this plugin, such as the implementation of pagination and max size limits, which help mitigate many risks. If we could adopt a similar approach here, it would be highly beneficial. Also, credit to Shargon for originally identifying this issue.

It seems that pagination is quite challenging here (credit to @Hecate2). Shargon's proposal might be a better approach. It would be more efficient to give users control over banning high-load methods, and we should set that as the default.

@shargon
Copy link
Member

shargon commented Feb 11, 2025

Because your two's claims are built on assumptions. Mine are built on facts.

The fact is: if you put this plugin in a consensus node, I can deny the services and kill one consensus node, I told you how can this be more secure, and you refuse to do the changes.

If you want to delegate in kestrel the cancellation and you call your own code, you must listen HttpContext.RequestAborted, otherwise the task will wait until finish your code to be cancelled.

@dusmart
Copy link

dusmart commented Feb 11, 2025

To ensure my understanding was correct, I also consulted with security expert @vang1ong7ang on this issue. His view is that DoS risks should be assessed based on the theoretical worst-case scenario. For example, the worst-case for the NEO's account API could return 100 million records, while for the GAS's account API, it could return 100 million * 100 million records. From this perspective, it should fundamentally be considered as having a DoS risk.

@cschuchardt88
Copy link
Member Author

All you concerns have been addressed.

@dusmart
Copy link

dusmart commented Feb 12, 2025

Thanks for your new commit! You made a great point—while applying page and size pagination at the outer layer reduces the returned data, it doesn’t necessarily decrease the internal CPU and memory load if the underlying processing remains the same (credit to @Hecate2). We suspect that this pagination approach might not be as effective as expected. @shargon, what are your thoughts?

@shargon
Copy link
Member

shargon commented Feb 12, 2025

Thanks for your new commit! You made a great point—while applying page and size pagination at the outer layer reduces the returned data, it doesn’t necessarily decrease the internal CPU and memory load if the underlying processing remains the same (credit to @Hecate2). We suspect that this pagination approach might not be as effective as expected. @shargon, what are your thoughts?

I think that we also need to be able to disable some endPoints, do it by controller it could be not enough

@cschuchardt88
Copy link
Member Author

We could add this too rate-limiting : https://learn.microsoft.com/en-us/aspnet/core/performance/rate-limit?view=aspnetcore-9.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants