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.

Sign up for GitHub

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

Jump to bottom

win,msi: use localized "Authenticated Users" name #39241

Merged
merged 1 commit into from
Jul 5, 2021
Merged

win,msi: use localized "Authenticated Users" name #39241

merged 1 commit into from
Jul 5, 2021

Conversation

richardlau
Copy link
Member

Well known user account names are localized on Windows. Look up the
"Authenticated Users" user by its security identifier to get the
localized name.

Refs: e817ba7
Refs: https://hackerone.com/reports/1211160
Fixes: #39224

cc @kumarak @nodejs/platform-windows

I think this will work, but I'm running on English Windows 10 so wasn't seeing the error reported on non-English locales. I'm going to build a test build so people can try out the installer.

@github-actions github-actions bot added install Issues and PRs related to the installers. needs-ci PRs that need a full CI run. tools Issues and PRs related to the tools directory. windows Issues and PRs related to the Windows platform. labels Jul 2, 2021
@richardlau
Copy link
Member Author

(For @nodejs/releasers, test build: https://ci-release.nodejs.org/job/iojs+release/7003/ )

@richardlau
Copy link
Member Author

Test installers

For anyone testing these please remember that Node.js installers do not allow multiple installations (I don't know the reasoning behind this) so running these installers will replace any version previously installed via installer. Since the updates are one way the only way to go back down to a release installer after installing these would be to completely uninstall first.

Having said that, if you are in a position to test these installers on a non-English locale please do so and feed back if they fix the issue in #39224. Bonus points if you can also verify the permissions of the installed directory as per https://hackerone.com/reports/1211160 (which is the security issue e817ba7 addressed).

I'm probably not going to be around much over the weekend, but if feedback to the test builds is positive we can probably spin out quick fix releases on Monday/Tuesday.

@targos
Copy link
Member

targos commented Jul 3, 2021

It should be possible to test it in isolation using the Windows Sandbox feature. I'll try on mine, it's localized in French

@targos
Copy link
Member

targos commented Jul 3, 2021 •
edited
Loading

I confirm that the install fails with v16.4.1 and works with the test build in a French sandbox.

unrelated issue

However, I tried to use npm, and that doesn't work well:

> npm i -g node-core-utils
(node:2052) MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 11 drain listeners added to [WriteStream]. Use emitter.setMaxListeners() to increase limit
(Use `node --trace-warnings ...` to show where the warning was created)
[..................] / reify: timing arborist:ctor Completed in 1ms

The command never ends. It works fine with v16.4.0 in the same environment.

@targos

This comment has been minimized.

Sign in to view
@targos targos mentioned this pull request Jul 3, 2021
Closed
4 tasks
targos
targos approved these changes Jul 3, 2021
View reviewed changes
Copy link
Member

@targos targos left a comment

Choose a reason for hiding this comment

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

LGTM based on my tests. I cannot judge the diff.

@targos targos mentioned this pull request Jul 3, 2021
Closed
@DvdGiessen
Copy link

DvdGiessen commented Jul 3, 2021 •
edited
Loading

Just ran some tests and this does indeed fix #39224. Installer no longer breaks, and I see the correct permissions are applied to the install directory. The approach taken in the diff looks correct to me.

(Note, I'm not sure MAX_PATH is the technically correct constant to use for defining the maximum length of the username/domainname, documentation seems to suggest that should be UNLEN+1/DNLEN+1 (from lmcons.h). But current code will work correctly, as MAX_PATH is defined as greater than either of those constants.)

@richardlau
Copy link
Member Author

Thanks @targos and @DvdGiessen! I've pushed an update to switch to UNLEN + 1/DNLEN + 1.

@Trott Trott mentioned this pull request Jul 3, 2021
Closed
@striezel
Copy link

striezel commented Jul 3, 2021 •
edited
Loading

Test installers

Just installed it on Windows 10 Home Edition, version 20H2, 64 bit, German language / locale. This seems to fix #39224.

Trott referenced this pull request Jul 3, 2021
@kumarak @richardlau
win,msi: set install directory permission
d0b449d 
Explicitly set permission for Windows install directory.

CVE-ID: CVE-2021-22921
Refs: https://hackerone.com/reports/1211160
PR-URL: nodejs-private/node-private#269
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/38856/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/38857/

@richardlau
win,msi: use localized "Authenticated Users" name
e9cf120 
Well known user account names are localized on Windows. Look up the
"Authenticated Users" user by its security identifier to get the
localized name.

PR-URL: nodejs#39241
Fixes: nodejs#39224
Refs: nodejs@e817ba7
Refs: https://hackerone.com/reports/1211160
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Beth Griggs <bgriggs@redhat.com>
@richardlau richardlau merged commit e9cf120 into nodejs:master Jul 5, 2021
@richardlau richardlau deleted the msbuild branch July 5, 2021 11:57
@richardlau
Copy link
Member Author

Landed in e9cf120

richardlau added a commit to richardlau/node-1 that referenced this pull request Jul 5, 2021
@richardlau
win,msi: use localized "Authenticated Users" name
182f86a 
Well known user account names are localized on Windows. Look up the
"Authenticated Users" user by its security identifier to get the
localized name.

PR-URL: nodejs#39241
Fixes: nodejs#39224
Refs: nodejs@e817ba7
Refs: https://hackerone.com/reports/1211160
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Beth Griggs <bgriggs@redhat.com>
@richardlau richardlau mentioned this pull request Jul 5, 2021
Merged
richardlau added a commit that referenced this pull request Jul 5, 2021
@richardlau
win,msi: use localized "Authenticated Users" name
0f00104 
Well known user account names are localized on Windows. Look up the
"Authenticated Users" user by its security identifier to get the
localized name.

PR-URL: #39241
Fixes: #39224
Refs: e817ba7
Refs: https://hackerone.com/reports/1211160
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Beth Griggs <bgriggs@redhat.com>
@richardlau richardlau mentioned this pull request Jul 5, 2021
Merged
BethGriggs pushed a commit that referenced this pull request Jul 5, 2021
@richardlau @BethGriggs
win,msi: use localized "Authenticated Users" name
76e709e 
Well known user account names are localized on Windows. Look up the
"Authenticated Users" user by its security identifier to get the
localized name.

PR-URL: #39241
Fixes: #39224
Refs: e817ba7
Refs: https://hackerone.com/reports/1211160
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Beth Griggs <bgriggs@redhat.com>
@BethGriggs BethGriggs mentioned this pull request Jul 5, 2021
Merged
ejose19 pushed a commit to ejose19/node that referenced this pull request Jul 5, 2021
@richardlau @ejose19
win,msi: use localized "Authenticated Users" name
883dbce 
Well known user account names are localized on Windows. Look up the
"Authenticated Users" user by its security identifier to get the
localized name.

PR-URL: nodejs#39241
Fixes: nodejs#39224
Refs: nodejs@e817ba7
Refs: https://hackerone.com/reports/1211160
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Beth Griggs <bgriggs@redhat.com>
@rriizzoo
Copy link

rriizzoo commented Jul 7, 2021

j

foxxyz pushed a commit to foxxyz/node that referenced this pull request Oct 18, 2021
@richardlau @foxxyz
win,msi: use localized "Authenticated Users" name
f244dc0 
Well known user account names are localized on Windows. Look up the
"Authenticated Users" user by its security identifier to get the
localized name.

PR-URL: nodejs#39241
Fixes: nodejs#39224
Refs: nodejs@e817ba7
Refs: https://hackerone.com/reports/1211160
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Beth Griggs <bgriggs@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BethGriggs BethGriggs BethGriggs approved these changes

@targos targos targos approved these changes

Assignees
No one assigned
Labels
install Issues and PRs related to the installers. needs-ci PRs that need a full CI run. tools Issues and PRs related to the tools directory. windows Issues and PRs related to the Windows platform.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Install directory permissions broken on non-English Windows systems
7 participants
@richardlau @targos @DvdGiessen @striezel @nodejs-github-bot @rriizzoo @BethGriggs