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

Calva uses Buffer(), which has been deprecated for security. #1939

Open
skylize opened this issue Nov 3, 2022 · 7 comments
Open

Calva uses Buffer(), which has been deprecated for security. #1939

skylize opened this issue Nov 3, 2022 · 7 comments
Labels
dependencies Pull requests that update a dependency file

Comments

@skylize
Copy link
Contributor

skylize commented Nov 3, 2022

Whenever Calva is enabled. The following warning appears in Dev Tools.

[Extension Host] (node:350285) [DEP0005] DeprecationWarning: Buffer() is deprecated due to security and usability issues. Please use the Buffer.alloc(), Buffer.allocUnsafe(), or Buffer.from() methods instead.
(Use `code --trace-deprecation ...` to show where the warning was created)
@skylize
Copy link
Contributor Author

skylize commented Nov 3, 2022

This issue explains why Buffer is unsafe.
nodejs/node#4660

And the deprecating commit.
nodejs/node-eps#4

Seems we are at fairly low risk, since not providing an http service, but I expect we are still potentially exposed to remote actors through Live Share. The fact this was deprecated all the way back in 2016 points to likely relying on dependencies that are not being maintained.

@PEZ
Copy link
Collaborator

PEZ commented Nov 3, 2022

npm audit doesn't bring this up. Just noting this as one of the clues.

@skylize
Copy link
Contributor Author

skylize commented Nov 3, 2022

After eliminating markdown files, and files that were clearly tests or examples (based on path and/or file name), I found occurrences of Buffer(...) or new Buffer(...) in the following files from Node modules folder.

No idea which of these are actually in Calva's code path, accept that:

I presume safe-buffer and safer-buffer are probably just calling their own replacement constructors, and whichever modules depend on them should probably be in the clear. I seriously doubt Mocha is in the code path for a normal run. And why on earth do we depend on Azure DevOps API?

Occurences
@webassemblyjs/leb128/esm/bufs.js
@webassemblyjs/leb128/lib/bufs.js
applicationinsights/out/Library/Channel.js
applicationinsights/out/Library/Util.js
azure-devops-node-api/FileContainerApi.js
azure-devops-node-api/opensource/node-http-ntlm/ntlm.js
azure-devops-node-api/WebApi.js
binary/index.js
binary/index.js
binary/perf/loop.js
binary/perf/small.js
buffer-crc32/index.js
buffer-from/index.js
buffer-indexof-polyfill/init-buffer.js
buffer-xor/index.js
buffer/index.js
buffers/index.js
coffeescript/lib/coffee-script/coffee-script.js
coffeescript/lib/coffee-script/repl.js
create-ecdh/browser.js
deep-extend/lib/deep-extend.js
diffie-hellman/lib/dh.js
envinfo/dist/envinfo.js
fd-slicer/index.js
fs-minipass/index.js
fstream/lib/collect.js
iconv-lite/lib/extend-node.js
ip/lib/ip.js
jszip/dist/jszip.js
jszip/dist/jszip.min.js
jszip/lib/nodejsUtils.js
lodash.isempty/index.js
lodash.isequal/index.js
lodash/isBuffer.js
lodash/lodash.js
memory-fs/lib/MemoryFileSystem.js
mocha/mocha-es2018.js
mocha/mocha.js
mocha/mocha.js.map
msgpack-lite/dist/msgpack.min.js
msgpack-lite/lib/bufferish-buffer.js
nerdbank-streams/MultiplexingStreamFormatters.js
nerdbank-streams/Utilities.js
node-addon-api/napi-inl.h
node-addon-api/napi.h
node-int64/Int64.js
prettier/bin-prettier.js
prettier/index.js
readline-sync/lib/readline-sync.js
ripemd160/index.js
safe-buffer/index.js
safer-buffer/dangerous.js
safer-buffer/safer.js
stream-http/lib/request.js
stream-http/lib/response.js
tunnel/lib/tunnel.js
typed-rest-client/opensource/Node-SMB/lib/ntlm.js
typescript/lib/tsc.js
typescript/lib/tsserver.js
typescript/lib/tsserverlibrary.js
typescript/lib/typescript.js
typescript/lib/typescriptServices.js
typescript/lib/typingsInstaller.js
unzipper/lib/Buffer.js
vscode-debugadapter/lib/protocol.js
yauzl/index.js
yazl/index.js

@PEZ
Copy link
Collaborator

PEZ commented Nov 3, 2022

The warning comes from a use of yauzl.

~(:|✔) % export NODE_OPTIONS=--throw-deprecation
~(:|✔) % code-exploration --install-extension betterthantomorrow.calva
Found 'betterthantomorrow.calva' in the marketplace.
Installing...
internal/process/warning.js:143
        throw warning;
        ^

DeprecationWarning: Buffer() is deprecated due to security and usability issues. Please use the Buffer.alloc(), Buffer.allocUnsafe(), or Buffer.from() methods instead.
    at showFlaggedDeprecation (buffer.js:160:11)
    at new Buffer (buffer.js:175:3)
    at fromRandomAccessReader (/private/var/folders/t5/gqxhj8pd6p9_tnvy6sbtmy480000gn/T/AppTranslocation/981FAF57-B42C-4DE5-B463-7F274138370A/d/Visual Studio Code - Exploration.app/Contents/Resources/app/node_modules.asar/yauzl/index.js:103:16)
    at /private/var/folders/t5/gqxhj8pd6p9_tnvy6sbtmy480000gn/T/AppTranslocation/981FAF57-B42C-4DE5-B463-7F274138370A/d/Visual Studio Code - Exploration.app/Contents/Resources/app/node_modules.asar/yauzl/index.js:55:5
    at FSReqWrap.oncomplete (fs.js:155:5)

From what I can see extract-zip and vsce uses yauzl. The latter is not involved when Calva is running, leaving extract-zip. I don't get this warning in dev mode of Calva, so it is a bit tricky experiment and verify this. But maybe building a vsix that doesn't use extract-zip (we only use it for extracting the clojure-lsp server) could verify this for us.

It seems extract-zip is poorly maintained. We are using the latest version and there are very old issues without answers, one of which mentions yauzl. We also use jszip, so maybe we can extract clojure-lsp using that one instead. If we conclude that extract-zip is the problem.

Care to work with this, @skylize?

@bpringe
Copy link
Member

bpringe commented Nov 5, 2022

The above is worth a try, though I think I've seen this warning since well before we added clojure-lsp or the downloading of it.

@PEZ
Copy link
Collaborator

PEZ commented Nov 5, 2022

This warning used to come from VS Code itself using yauzl. They fixed that some year ago, if I understand the issues correctly.

@skylize
Copy link
Contributor Author

skylize commented Nov 18, 2022

Since it has been a couple weeks without progress, I think it is worth an explicit answer here that I do intend to work on this.

@bpringe bpringe added the dependencies Pull requests that update a dependency file label Nov 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Development

No branches or pull requests

3 participants