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

Electron 21+ breaks sodium_malloc #185

Closed
mixmix opened this issue Oct 5, 2023 · 8 comments
Closed

Electron 21+ breaks sodium_malloc #185

mixmix opened this issue Oct 5, 2023 · 8 comments

Comments

@mixmix
Copy link
Contributor

mixmix commented Oct 5, 2023

Trying to upgrade to electron@26 , I started seeing an error which I tracked back to

sodium-native/blob/v3.4.1/binding.c#L95

SN_STATUS_THROWS(napi_create_external_buffer(env, size, ptr, NULL, NULL, &buf), "failed to create a n-api buffer")

Some research surfaces this issue electron/electron#35801

TL;DR - the new memory cage in v8 deprecated napi_create_external_buffer 👍

The electron issue has people discussing different approaches to navigating this breaking change. I don't know enough of this domain (yet) to be able to take this further.

@mixmix
Copy link
Contributor Author

mixmix commented Oct 5, 2023

Electron 20.x.y has reached end-of-support ...

☑️ Last working version electron@20.3.8

@mixmix
Copy link
Contributor Author

mixmix commented Oct 10, 2023

@mafintosh / @emilbayes I manage the scuttlebutt maintenance fund and would love to contribute funds to this issue being resolved.

Are either of you interested (or can you recommend someone who you'd trust to merge/publish a fix?). I'm interested in version 3 being patched for the moment, but the same fix will be applicable to 4. I imagine this is relevant to Keet too?

@mafintosh
Copy link
Member

Dont think its fixable as electron disabled the api. We just dont use the secure mem atm. If I am wrong and it is fixable, then happy to apply.

@mixmix
Copy link
Contributor Author

mixmix commented Oct 10, 2023

@mafintosh I can see these paths forward, have marked this bits which I think you might be able to answer with (:question:).

If any seem of interest to explore, would be up for resourcing that (even if it's a dead end). If (1) is safe and fine, I can take that path instead

1. replace sodium_malloc with Buffer.alloc

  • ⚠️ less secure?
    • can you speak to this << ❓
  • 💉 modules I'd be changing:
    • noise-protocol (emile's)
    • hmac-blake2b (emile's)
    • artefact-store (mine)
    • ssb-private-group-keys (mine)

2. copy buffers into memory cage

This blog describes how to do this: electronjs.org/blog/v8-memory-cage#i-want-to-refactor-a-node-native...

This will copy the data into a newly-allocated memory region that is inside the V8 memory cage. Optionally, N-API can also provide a pointer to the newly-copied data, in case you need to modify or reference it after the fact.

  • ☑️ looks doable
  • 🐌 performance hit?
    • how to check << ❓
    • could check if cage is in place and pivot napi use based on that?
      • napi_no_external_buffers_allowed or NODE_API_NO_EXTERNAL_BUFFERS_ALLOWED (see docs)

3. fancy v8 memory allocation?

End of same blog as in (2) says:

Refactoring to use V8's memory allocator is a little more complicated, because it requires modifying the create_external_resource function to use memory allocated by V8, instead of using malloc. This may be more or less feasible, depending on whether or not you control the definition of create_external_resource. The idea is to first create the buffer using V8, e.g. with napi_create_buffer, and then initialize the resource into the memory that has been allocated by V8. It is important to retain a napi_ref to the Buffer object for the lifetime of the resource, otherwise V8 may garbage-collect the Buffer and potentially result in use-after-free errors.

  • 🤔 I don't know enough to comment
    • needs research / feasibility check << ❓

@mafintosh
Copy link
Member

  1. not up to sodium-native to make that decision, thats up to the consumer. we can expose a flag whether its enabled if we dont already. We simply moved away from secure buffers in our active modules for Hypercore cause its too painful in js unfortunately.
  2. might be misunderstanding the link, but if you copy the contents of the buffer the secure memory doesn't really do much for you, security wise.
  3. sodium needs to malloc the buffer here, unsure if that works.

@mixmix
Copy link
Contributor Author

mixmix commented Oct 12, 2023

(1) answers my question adequately I think - if Hypercore team is happy with Buffer.alloc for good enough security, then it's likely good enough for my needs too. There are probably easier ways to attack my projects that trying to access Buffers

Appreciate your time, thanks @mafintosh

@mixmix
Copy link
Contributor Author

mixmix commented Oct 25, 2023

Here's the solution I'm using. This is at the start of the main.js file that electron runs:

// WARNING - monkey patch
const na = require('sodium-native')
na.sodium_malloc = function sodium_malloc_monkey_patched (n) {
  return Buffer.alloc(n)
}

🙈

@mixmix
Copy link
Contributor Author

mixmix commented Oct 31, 2023

Ah, and you also need

na.sodium_free = function sodium_free_monkey_patched () {}

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

No branches or pull requests

2 participants