src: call linux getauxval(AT_SECURE) in SafeGetenv #12548

danbev · 2017-04-20T18:18:44Z

This commit attempts to fix the following TODO:

// TODO(bnoordhuis) Should perhaps also check whether getauxval(AT_SECURE) is non-zero on Linux.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

src

danbev · 2017-04-20T18:19:25Z

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

cjihrig

LGTM if the CI is happy... but it appears not to be so far.

bnoordhuis · 2017-04-20T19:28:16Z

I'm afraid you can't use plain getauxval() because it is a fairly recent addition to glibc (>=2.16) and we support (much) older versions.

You can get the value of AT_SECURE by rooting through the auxiliary vector that the kernel puts after the programs arguments and the environment but that is complicated by the relocating of argv and environ that uv_setup_args() does right after startup.

Perhaps the best option is to do it as early as possible, in main() in src/node_main.cc. Doing it in Start() won't work because it could be an embedder-provided argv vector.

EDIT: I owe you an apology for the comment. I was using "call getauxval(AT_SECURE)" as a shorthand for "call our lovingly handcrafted homegrown getauxval(AT_SECURE) reimplementation that libc does not provide" but there is no way you could have guessed that from looking at it.

Fishrock123 · 2017-04-20T19:59:15Z

Maybe we can use this if node is compiled with a version that supports it? idk what our guidelines would be for #if statements like that. (I think that is possible to begin with...)

danbev · 2017-04-21T06:59:46Z

Maybe we can use this if node is compiled with a version that supports it? idk what our guidelines would be for #if statements like that. (I think that is possible to begin with...)

I'll try this and see if I can get that right and have it pass the CI tests. Let me know if this is what you hade in mind.

danbev · 2017-04-21T07:01:07Z

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

danbev · 2017-04-21T07:10:09Z

I was using "call getauxval(AT_SECURE)" as a shorthand for "call our lovingly handcrafted homegrown getauxval(AT_SECURE)

Just trying to see if I understand what should happen here. We need to retrieve the auxiliary vector information in node_main, and store the key value pairs so that when the node version of getauxval is called the value can be returned?

bnoordhuis · 2017-04-21T08:06:07Z

idk what our guidelines would be for #if statements like that.

Not allowed, but dynamic detection with dlsym() is.

Rationale: a binary compiled on a system with getauxval() would be unusable on a system without.

We need to retrieve the auxiliary vector information in node_main, and store the key value pairs so that when the node version of getauxval is called the value can be returned?

Correct. You don't need to copy out the aux vector (libuv doesn't touch it) but neither would it hurt.

danbev · 2017-04-24T12:57:45Z

Correct. You don't need to copy out the aux vector (libuv doesn't touch it) but neither would it hurt.

Thanks for the verifying. I've made an attempt at this now. It lacks any form of tests but if this looks like I'm on the right path I'll take a look at adding something. I've just manually tested using the following steps for now:

$ setcap cap_net_raw+ep out/Release/node
$ NODE_PENDING_DEPRECATION="1" out/Release/node -p "process.binding('config').pendingDeprecation"
true
$ useradd test
$ su test
$ NODE_PENDING_DEPRECATION="1" out/Release/node -p "process.binding('config').pendingDeprecation"
undefined

danbev · 2017-04-24T12:58:40Z

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

danbev · 2017-05-06T12:27:01Z

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

danbev · 2017-05-17T12:28:01Z

Rebased CI: https://ci.nodejs.org/job/node-test-pull-request/8117/
I want to trigger the failures again to try to figure out the issues.

danbev · 2017-05-18T05:49:43Z

test/arm seems to have completed successfully

https://ci.nodejs.org/job/node-test-commit-arm/nodes=armv7-ubuntu1404/9741/console:

Run condition [Always] enabling perform for step [[Execute a set of scripts, Execute a set of scripts]]
TAP Reports Processing: START
Looking for TAP results report in workspace using pattern: *.tap
Saving reports...
Processing '/var/lib/jenkins/jobs/node-test-commit-arm/configurations/axis-nodes/armv7-ubuntu1404/builds/9741/tap-master-files/cctest.tap'
Parsing TAP test result [/var/lib/jenkins/jobs/node-test-commit-arm/configurations/axis-nodes/armv7-ubuntu1404/builds/9741/tap-master-files/cctest.tap].
Processing '/var/lib/jenkins/jobs/node-test-commit-arm/configurations/axis-nodes/armv7-ubuntu1404/builds/9741/tap-master-files/test.tap'
Parsing TAP test result [/var/lib/jenkins/jobs/node-test-commit-arm/configurations/axis-nodes/armv7-ubuntu1404/builds/9741/tap-master-files/test.tap].
TAP Reports Processing: FINISH
Checking ^not ok
Notifying upstream projects of job completion
Finished: SUCCESS

danbev · 2017-05-18T05:51:58Z

@bnoordhuis Could you take another look at this when you get a chance?

bnoordhuis · 2017-05-18T08:01:48Z

src/node.cc

+#if defined(__linux__)
+AuxVector::AuxVector() {
+  char** envp = environ;
+  while (*envp++ != NULL) {}


I'll fix this.

bnoordhuis · 2017-05-18T08:17:37Z

src/node.cc

+  Elf_auxv_t* auxv = reinterpret_cast<Elf_auxv_t*>(envp);
+  for (; auxv->a_type != AT_NULL; auxv++) {
+    auxv_map_[auxv->a_type] = auxv->a_un.a_val;
+  }


You should skip AT_IGNORE entries here. That said, I'd make this the simplest possible implementation and not bother with a map since we're only interested in AT_SECURE. E.g. bool linux_at_secure; in node.cc in and set it in node_main.cc if AT_SECURE is present (with an extern bool linux_at_secure; declaration to make it link.)

I'd be okay with unconditionally defining the bool to save on #ifdef __linux__ ugliness.

That sounds much better. I'll give that a go and see what you think.

bnoordhuis · 2017-05-18T08:18:08Z

src/node.h

+  void operator=(AuxVector const&);
+  std::map<std::size_t, std::size_t> auxv_map_;
+};
+#endif


This should not go into node.h, it's a public header. node_internals.h is okay, though.

I'll move it. Thanks

This commit attempts to fix the following TODO: // TODO(bnoordhuis) Should perhaps also check whether getauxval(AT_SECURE) is non-zero on Linux. This can be manually tested at the moment using the following steps: $ setcap cap_net_raw+ep out/Release/node $ NODE_PENDING_DEPRECATION="1" out/Release/node -p "process.binding('config').pendingDeprecation" true $ useradd test $ su test $ NODE_PENDING_DEPRECATION="1" out/Release/node -p "process.binding('config').pendingDeprecation" undefined

danbev · 2017-05-22T13:36:17Z

@bnoordhuis I've made another attempt on this and tried to follow your suggestions. Could you take another look and see if this was what you had in mind? Thanks

bnoordhuis

LGTM modulo nit and question. Nice work.

bnoordhuis · 2017-05-24T13:58:42Z

src/node_main.cc

+#else
+#define Elf_auxv_t Elf32_auxv_t
+#endif  // __LP64__
+extern char **environ;


Style nit: extern char** environ;

I'll fix this.

bnoordhuis · 2017-05-24T14:01:11Z

src/node_main.cc

@@ -71,7 +71,33 @@ int wmain(int argc, wchar_t *wargv[]) {
 }
 #else
 // UNIX
+#ifdef __linux__
+#include <elf.h>
+#include <inttypes.h>


What do you need inttypes.h for?

danbev · 2017-05-24T18:01:23Z

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

This commit attempts to fix the following TODO: // TODO(bnoordhuis) Should perhaps also check whether getauxval(AT_SECURE) is non-zero on Linux. This can be manually tested at the moment using the following steps: $ setcap cap_net_raw+ep out/Release/node $ NODE_PENDING_DEPRECATION="1" out/Release/node -p "process.binding('config').pendingDeprecation" true $ useradd test $ su test $ NODE_PENDING_DEPRECATION="1" out/Release/node -p "process.binding('config').pendingDeprecation" undefined PR-URL: nodejs#12548 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

danbev · 2017-05-25T17:10:40Z

Landed in 6caf1b0

This commit attempts to fix the following TODO: // TODO(bnoordhuis) Should perhaps also check whether getauxval(AT_SECURE) is non-zero on Linux. This can be manually tested at the moment using the following steps: $ setcap cap_net_raw+ep out/Release/node $ NODE_PENDING_DEPRECATION="1" out/Release/node -p "process.binding('config').pendingDeprecation" true $ useradd test $ su test $ NODE_PENDING_DEPRECATION="1" out/Release/node -p "process.binding('config').pendingDeprecation" undefined PR-URL: #12548 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

MylesBorins · 2017-07-17T16:43:54Z

This does not land cleanly in LTS. Please feel free to manually backport. Please also feel free to replace the backport request label with do-not-land if it shouldn't land

This commit suggests using getauxval in node_main.cc. The motivation for this is that getauxval was introduced in glibc 2.16 and looking at BUILDING.md, in the 'Platform list' section, it looks like we now support glibc >= 2.17 and perhaps this change would be alright now. Refs: nodejs#12548

This commit suggests using getauxval in node_main.cc. The motivation for this is that getauxval was introduced in glibc 2.16 and looking at BUILDING.md, in the 'Platform list' section, it looks like we now support glibc >= 2.17 and perhaps this change would be alright now. PR-URL: #33693 Refs: #12548 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: David Carlier <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Apr 20, 2017

cjihrig approved these changes Apr 20, 2017

View reviewed changes

danbev force-pushed the getauxval_at_secure branch from f4e6fa2 to b356a19 Compare April 24, 2017 12:53

danbev force-pushed the getauxval_at_secure branch from b356a19 to 914d196 Compare May 5, 2017 08:27

danbev force-pushed the getauxval_at_secure branch from c3842c2 to f7636a5 Compare May 17, 2017 12:25

danbev force-pushed the getauxval_at_secure branch from e5bbee7 to 848294b Compare May 18, 2017 03:49

bnoordhuis requested changes May 18, 2017

View reviewed changes

danbev force-pushed the getauxval_at_secure branch from 848294b to a7c7bea Compare May 18, 2017 13:30

bnoordhuis approved these changes May 24, 2017

View reviewed changes

danbev added 2 commits May 24, 2017 19:59

remove unused inttypes.h

21f2446

fix pointer style

e4eb855

danbev closed this May 25, 2017

danbev deleted the getauxval_at_secure branch May 25, 2017 17:10

danbev mentioned this pull request May 26, 2017

src: clean up SafeGetenv() #13220

Merged

2 tasks

jasnell mentioned this pull request May 28, 2017

8.0.0 Release Proposal #12220

Closed

gibfahn mentioned this pull request Jun 15, 2017

Auditing for 6.11.1 nodejs/Release#230

Closed

3 tasks

MylesBorins added the backport-requested-v6.x label Jul 17, 2017

danbev mentioned this pull request Jun 2, 2020

src: use getauxval in node_main.cc #33693

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: call linux getauxval(AT_SECURE) in SafeGetenv #12548

src: call linux getauxval(AT_SECURE) in SafeGetenv #12548

danbev commented Apr 20, 2017

danbev commented Apr 20, 2017

cjihrig left a comment

bnoordhuis commented Apr 20, 2017 •

edited

Loading

Fishrock123 commented Apr 20, 2017

danbev commented Apr 21, 2017

danbev commented Apr 21, 2017

danbev commented Apr 21, 2017

bnoordhuis commented Apr 21, 2017

danbev commented Apr 24, 2017

danbev commented Apr 24, 2017

danbev commented May 6, 2017

danbev commented May 17, 2017

danbev commented May 18, 2017

danbev commented May 18, 2017

bnoordhuis May 18, 2017

danbev May 18, 2017

bnoordhuis May 18, 2017

danbev May 18, 2017

bnoordhuis May 18, 2017

danbev May 18, 2017

danbev commented May 22, 2017

bnoordhuis left a comment

bnoordhuis May 24, 2017

danbev May 24, 2017

bnoordhuis May 24, 2017

danbev commented May 24, 2017

danbev commented May 25, 2017

MylesBorins commented Jul 17, 2017

src: call linux getauxval(AT_SECURE) in SafeGetenv #12548

src: call linux getauxval(AT_SECURE) in SafeGetenv #12548

Conversation

danbev commented Apr 20, 2017

Checklist

Affected core subsystem(s)

danbev commented Apr 20, 2017

cjihrig left a comment

Choose a reason for hiding this comment

bnoordhuis commented Apr 20, 2017 • edited Loading

Fishrock123 commented Apr 20, 2017

danbev commented Apr 21, 2017

danbev commented Apr 21, 2017

danbev commented Apr 21, 2017

bnoordhuis commented Apr 21, 2017

danbev commented Apr 24, 2017

danbev commented Apr 24, 2017

danbev commented May 6, 2017

danbev commented May 17, 2017

danbev commented May 18, 2017

danbev commented May 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danbev commented May 22, 2017

bnoordhuis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danbev commented May 24, 2017

danbev commented May 25, 2017

MylesBorins commented Jul 17, 2017

bnoordhuis commented Apr 20, 2017 •

edited

Loading