-
Notifications
You must be signed in to change notification settings - Fork 118
input validation 3 #662
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
base: main
Are you sure you want to change the base?
input validation 3 #662
Conversation
… are all in rust lol
I think cargo deny is dumb so I am going to drop it. |
For the sake of this PR, please ignore the safeunmanagedvector. I think it is immature but haven't stripped it out yet. |
…to faddat/input-validation-3
We're now using wasmver v6.0.x here, thanks @chipshort |
You may be wondering why I've included the upgrade to go v1.24 and wasmer v6 here -- the gist of it is I think both are implicated in different aspects of problematic performance that can degrade ux and security, so it makes sense to keep with the latest items. Together, all of these changes should be seen as a security fix. I don't think they are very meaningful when looked at separately, except for the input validation stuff, which I think it's best not to dig into so much and instead just get fixed. I am using a fork of cosmwasm, the branch is here: There's also a PR here: I don't know how bad all this is but I'll make my public speculations on Wednesday. Before then, I am working to fix as many issues from this issue class as possible, and I intend to release a patched (heh really just linted) wasmvm to any and all affected teams on Wednesday, and the affected teams are all teams. Actually I just re-read that and tbh I have no clue what the hell to do, like about releasing a patch and stuff. I've been talking to ICL about these issues since October 2024, and from where I stand as a normal flawed dude who can make mistakes, I'm pretty sure that these are really serious issues. It's funny cause I'm pretty sure no one cares, guess I could be wrong? If anyone sees this PR and feels that the issues are NOT serious, can you please lmk why? |
Hey I'll get this back on mainline cosmwasm and wasmer. That change was too fancy of me. |
t.Logf("Time (%d gas): %s\n", cost.UsedInternally, diff) | ||
|
||
// make sure it read the balance properly and we got 250 atoms | ||
var result types.ContractResult | ||
err = json.Unmarshal(res, &result) | ||
require.NoError(t, err) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I.... definitely need to fix this
internal/api/lib_test.go
Outdated
|
||
var result types.ContractResult | ||
err = json.Unmarshal(res, &result) | ||
require.NoError(t, err) | ||
// If we get a validation error, that's ok for this test - just ignore it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also definitely needs to be fixed
Key Changes:
SafeByteSlice
,SafeUnmanagedVector
, and memory operation validations to stop memory-related issues.How It’s Done:
into_result_safe
keeps things smooth.Testing Updates:
CreateChecksum
(nil, empty, short, invalid magic, and valid WASM scenarios).Let’s Talk:
I’m a bit unsure about how serious some of these issues are—your thoughts? Discussions with ICL have been rolling since October 2024, and a patched version is in the works. Please review these changes and share your feedback. Your input will help make this as solid as possible!