[WASI] WASI Random Plugin #2875

LFsWang · 2023-09-24T13:54:47Z

add WASI Random Plugin

Progress

random
insecure
insecure-seed

Support

Linux
MacOS
Windows

juntao · 2023-09-24T13:54:49Z

Hello, I am a code review bot on flows.network. Here are my reviews of code commits in this PR.

Commit 81f8220a28eb71105b068071d9042b2e3f06c5b2

Key Changes:

The pull request appears to add a new plugin wasi_random, which provides an implementation for getting random bytes on different platforms (Linux, MacOS, Windows), for the WebAssembly System Interface (WASI).

Specifically, the changes include:

Several modifications to the build-extensions.yml. This file serves for building of all the existing plugins. The new plugin, wasi_random has been added in several build options variables, tar names, test names, and output binaries - essentially configuring the GitHub actions for building the new plugin.
Several implementations of this wasi_random plugin have been added, specifically for Linux, Windows, and MacOS systems. These essentially include functionalities for acquiring random bytes.
New files have been created which include Cmake configurations and the new code for the random generator plugin.
The Cmake files configure wasi_random as a shared library and define the dependencies for the plugin. The wasi_random plugin linkage and testing file organization have been added in the root plugin cmake file.
To support Windows, a few declarations are made to declare Windows API for Crypto functions dealing with randomness.
In the tests directory, new files for testing this plugin have also been added.

Potential Issues:

It's difficult to find potential issues without the context of actual code changes. However, one potential issue to look out for would be security or privacy. When handling random numbers, especially those used for cryptographic uses, it would be important to ensure that the operations are secure and free from any possible security vulnerability or privacy leaks.
Ensure that wasi_random is compatible with the rest of the WASI ecosystem.
The platform-specific implementations for Linux, Windows, and MacOS should be thoroughly tested to work correctly on those platforms.
The case when the OS random byte getter is not available or fails is not handled. This might lead to issues.
No documentation has been added, this will make it hard for others to understand how to use and what is the purpose of wasi_random.
Testing will be needed to ensure that the API behaves correctly for all possible inputs and states. The Tests provided in the patch should provide a good level of coverage, but boundary or edge cases might be missing.
The efficiency of the plugin hasn’t been discussed or tested.

codecov · 2023-09-24T14:09:59Z

Codecov Report

Merging #2875 (2e07267) into master (88d4922) will increase coverage by 0.01%.
Report is 2 commits behind head on master.
The diff coverage is n/a.

❗ Current head 2e07267 differs from pull request most recent head 81f8220. Consider uploading reports for the commit 81f8220 to get more accurate results

@@            Coverage Diff             @@
##           master    #2875      +/-   ##
==========================================
+ Coverage   80.94%   80.96%   +0.01%     
==========================================
  Files         150      150              
  Lines       21624    21624              
  Branches     4352     4352              
==========================================
+ Hits        17503    17507       +4     
  Misses       2954     2954              
+ Partials     1167     1163       -4

see 3 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

dannypsnl · 2023-09-25T09:12:05Z

NOTE glibc's LCG is insufficient for cryptography.

Signed-off-by: Sylveon <sylveon@secondstate.io>

dannypsnl · 2023-10-03T05:53:56Z

plugins/wasi_random/details/random_linux.cpp

+bool WasiRandomEnvironment::getRandomBytes(uint64_t Len, uint8_t *Buf) {
+  ssize_t RealQuerySize =
+      getrandom_wrapper(Buf, Len, GRND_RANDOM | GRND_NONBLOCK);
+
+  // note: the spec did not handle the error such like EAGAIN, we just make that
+  // case fail.
+  if (RealQuerySize < 0 || RealQuerySize != static_cast<ssize_t>(Len))
+    return false;
+
+  return true;
+}
+
+bool WasiRandomEnvironment::getInsecureRandomBytes(uint64_t Len, uint8_t *Buf) {
+  ssize_t RealQuerySize =
+      getrandom_wrapper(Buf, Len, GRND_INSECURE | GRND_NONBLOCK);
+
+  // note: the spec did not handle the error such like EAGAIN, we just make that
+  // case fail.
+  if (RealQuerySize < 0 || RealQuerySize != static_cast<ssize_t>(Len))
+    return false;
+
+  return true;
+}


Why not merge them? If they're just with different flag.

dannypsnl · 2023-10-03T05:54:37Z

plugins/wasi_random/details/random_macos.cpp

+bool WasiRandomEnvironment::getRandomBytes(uint64_t Len, uint8_t *Buf) {
+  auto Status = CCRandomGenerateBytes(Buf, Len);
+  return Status == kCCSuccess;
+}
+
+bool WasiRandomEnvironment::getInsecureRandomBytes(uint64_t Len, uint8_t *Buf) {
+  auto Status = CCRandomGenerateBytes(Buf, Len);
+  return Status == kCCSuccess;
+}


They're exact same, right?

dannypsnl · 2023-10-03T05:55:10Z

plugins/wasi_random/details/random_windows.cpp

+bool WasiRandomEnvironment::getRandomBytes(uint64_t Len, uint8_t *Buf) {
+  if (Len > UINT32_MAX)
+    return false;
+  return WinGetRandomBytes(static_cast<uint32_t>(Len), Buf);
+}
+
+bool WasiRandomEnvironment::getInsecureRandomBytes(uint64_t Len, uint8_t *Buf) {
+  if (Len > UINT32_MAX)
+    return false;
+  return WinGetRandomBytes(static_cast<uint32_t>(Len), Buf);
+}


Similar problem

dannypsnl · 2023-10-03T05:57:37Z

plugins/wasi_random/func.cpp

+  if (unlikely(Env.getRandomBytes(Len, Buf.data()) == false)) {
+    return Unexpect(ErrCode::Value::HostFuncError);
+  }
+  return {};
+}
+
+Expect<void> WasiGetInsecureRandomU64::body(const Runtime::CallingFrame &Frame,
+                                            uint32_t /* Out */ U64Ptr) {
+  // Check memory instance from module.
+  auto *MemInst = Frame.getMemoryByIndex(0);
+  if (unlikely(MemInst == nullptr)) {
+    return Unexpect(ErrCode::Value::HostFuncError);
+  }
+
+  // Check for invalid address.
+  constexpr auto U64Len = sizeof(uint64_t);
+  const auto Buf = MemInst->getSpan<uint8_t>(U64Ptr, U64Len);
+  if (unlikely(Buf.size() != U64Len)) {
+    return Unexpect(ErrCode::Value::HostFuncError);
+  }
+
+  if (unlikely(Env.getRandomBytes(U64Len, Buf.data()) == false)) {


Are them going to use getInsecureRandomBytes? Or intended use getRandomBytes?

dannypsnl · 2023-10-03T05:58:53Z

plugins/wasi_random/module.cpp

+namespace Host {
+
+using namespace std::literals;
+


Maybe leave a link to the preview2, where one can check the WIT definitions.

LFsWang requested a review from ibmibmibm as a code owner September 24, 2023 13:54

github-actions bot added the c-Plugin An issue related to WasmEdge Plugin label Sep 24, 2023

LFsWang force-pushed the wasi-random branch 2 times, most recently from c80813d to 27c9782 Compare September 24, 2023 13:58

LFsWang force-pushed the wasi-random branch from 27c9782 to f1805e1 Compare September 24, 2023 14:40

LFsWang requested review from q82419 and hydai as code owners September 24, 2023 14:40

github-actions bot added c-Test An issue/PR to enhance the test suite c-CI labels Sep 24, 2023

LFsWang force-pushed the wasi-random branch 5 times, most recently from 7711d1a to 2e07267 Compare September 25, 2023 08:52

LFsWang force-pushed the wasi-random branch from 2e07267 to 38df4ee Compare September 25, 2023 10:08

[WASI] add wasi_random as plugin

81f8220

Signed-off-by: Sylveon <sylveon@secondstate.io>

LFsWang force-pushed the wasi-random branch from 38df4ee to 81f8220 Compare September 25, 2023 10:09

dannypsnl mentioned this pull request Sep 27, 2023

Track WASI Preview 2 Status #2806

Closed

8 tasks

dannypsnl requested changes Oct 3, 2023

View reviewed changes

hydai mentioned this pull request Oct 3, 2023

WASI preview2 proposals tracking issue #2939

Open

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WASI] WASI Random Plugin #2875

[WASI] WASI Random Plugin #2875

LFsWang commented Sep 24, 2023 •

edited

juntao commented Sep 24, 2023 •

edited

codecov bot commented Sep 24, 2023 •

edited

dannypsnl commented Sep 25, 2023

dannypsnl Oct 3, 2023

dannypsnl Oct 3, 2023

dannypsnl Oct 3, 2023

dannypsnl Oct 3, 2023

dannypsnl Oct 3, 2023

[WASI] WASI Random Plugin #2875

Are you sure you want to change the base?

[WASI] WASI Random Plugin #2875

Conversation

LFsWang commented Sep 24, 2023 • edited

juntao commented Sep 24, 2023 • edited

Commit 81f8220a28eb71105b068071d9042b2e3f06c5b2

codecov bot commented Sep 24, 2023 • edited

Codecov Report

dannypsnl commented Sep 25, 2023

dannypsnl Oct 3, 2023

Choose a reason for hiding this comment

dannypsnl Oct 3, 2023

Choose a reason for hiding this comment

dannypsnl Oct 3, 2023

Choose a reason for hiding this comment

dannypsnl Oct 3, 2023

Choose a reason for hiding this comment

dannypsnl Oct 3, 2023

Choose a reason for hiding this comment

LFsWang commented Sep 24, 2023 •

edited

juntao commented Sep 24, 2023 •

edited

codecov bot commented Sep 24, 2023 •

edited