From a490ec966303248b0c131a4e19fda518caf87249 Mon Sep 17 00:00:00 2001 From: William Mantly Date: Sat, 18 Jul 2026 17:48:25 -0400 Subject: [PATCH] 2.0.1 --- docs/issues.md | 328 +++++++++++++++++++++++++++++++++++++++++++++++++ issue.md | 308 ++++++++++++++++++++++++++++++++++++++++++++++ package.json | 2 +- 3 files changed, 637 insertions(+), 1 deletion(-) create mode 100644 docs/issues.md create mode 100644 issue.md diff --git a/docs/issues.md b/docs/issues.md new file mode 100644 index 0000000..e135d7f --- /dev/null +++ b/docs/issues.md @@ -0,0 +1,328 @@ +# Issue Report: linux-sys-user + +This document details issues found during code review of the linux-sys-user module. + +--- + +## 1. Inconsistent Error Handling in `spawnWrapper` + +**Location**: `lib/user.js:26-31` + +**Problem**: The `spawnWrapper` function has inconsistent behavior between spawn errors and normal exit: + +```javascript +_p.on("error", function () { + callback(stderr || true, stdout); // Returns: (stderr || true, stdout) +}); +_p.on("exit", function () { + callback(stderr, stdout); // Returns: (stderr, stdout) +}); +``` + +When a spawn error occurs, `callback` is called with `stderr || true` - ensuring a truthy first argument. But on normal exit, `callback` receives `stderr` directly, which may be an empty string (falsy). + +**Impact**: +- Calling code cannot reliably distinguish between a spawn failure and a successful command that produced no stderr output. +- Functions that check `if (error) callback(error)` will behave inconsistently. + +**Recommendation**: Normalize error handling to always return a consistent format. Consider creating an `Error` object or using a consistent sentinel value: + +```javascript +_p.on("error", function (err) { + callback(new Error(err.message || "Process spawn failed"), stdout); +}); +``` + +--- + +## 2. TypeScript Type Mismatch for `selinux_user` + +**Location**: `lib/user.js:79-81` and `index.d.ts:11` + +**Problem**: The TypeScript definition types `selinux_user` as `boolean`, but the implementation passes it as a string argument to `useradd`: + +```javascript +// lib/user.js:79-81 +selinux_user: function (SEUSER) { + if (SEUSER) { + return ["--selinux-user", SEUSER]; // SEUSER is used as a string + } +}, +``` + +```typescript +// index.d.ts:11 +selinux_user?: boolean; // Typed as boolean +``` + +**Impact**: TypeScript users will receive type errors when passing a valid string value like `"staff_u"`. + +**Recommendation**: Fix the type definition: + +```typescript +selinux_user?: string; +``` + +--- + +## 3. Fragile Date Parsing in `getExpiration` + +**Location**: `lib/user.js:331-350` + +**Problem**: The `getExpiration` function parses `chage --list` output by splitting on newlines and colons, assuming a specific output format: + +```javascript +data = { + changed: new Date(data[0].split(": ")[1]), + passwordExpires: + data[1].split(": ")[1] === "never" + ? null + : new Date(data[1].split(": ")[1]), + // ... assumes data[0] through data[6] exist with exact format +}; +``` + +**Impact**: +- Output format may vary by `chage` version or system locale/language settings. +- If `data` array lacks expected indices, will throw `TypeError: Cannot read property 'split' of undefined`. +- Date strings in different locales may not parse correctly with `new Date()`. + +**Recommendation**: Add defensive checks and locale-aware parsing: + +```javascript +if (!data || data.length < 7) { + return callback(new Error("Unexpected chage output format")); +} + +var parseDate = function(str) { + if (!str || str.trim() === "never") return null; + // Handle locale-specific date formats if needed + return new Date(str); +}; +``` + +--- + +## 4. Unreliable Password Setting via Stdin + +**Location**: `lib/user.js:210-217` + +**Problem**: The `setPassword` function writes to `passwd`'s stdin but may not work reliably: + +```javascript +var _p = spawn("passwd", [username]); +_p.stdin.write(password + "\n"); +_p.stdin.write(password + "\n"); +_p.stdin.end(); +``` + +Some `passwd` implementations may not accept password via stdin, or may behave differently depending on PAM configuration, security policies, or whether stdin is a terminal. + +**Impact**: Password changes may silently fail or behave unexpectedly on some systems. + +**Recommendation**: Consider using `chpasswd` which is designed for non-interactive password input: + +```javascript +var _p = spawn("chpasswd", []); +_p.stdin.write(username + ":" + password + "\n"); +_p.stdin.end(); +``` + +Alternatively, document that this method requires specific system configuration to work. + +--- + +## 5. Potential Command Injection in `addSSHtoUser` + +**Location**: `lib/user.js:387-398` + +**Problem**: The `addSSHtoUser` function uses `exec` with a format string built from user data: + +```javascript +var commands = [ + util.format("mkdir %s/.ssh;", info.homedir), + util.format("touch %s/.ssh/authorized_keys;", info.homedir), + util.format("chown 700 %s/.ssh;", info.homedir), + util.format("chmod 600 %s/.ssh/authorized_keys;", info.homedir), + util.format("chown %s:%s %s/.ssh -R;", info.username, info.username, info.homedir), +].join(" "); +``` + +The comment claims this is safe because "we are not taking any user input data", but `info.homedir` comes from `/etc/passwd` parsing. + +**Impact**: If `/etc/passwd` is compromised or contains malicious data, command injection could occur. + +**Recommendation**: Validate paths before use: + +```javascript +var safePath = function(path) { + // Only allow absolute paths under /home or /root + if (!path || !path.match(/^\/(home|root)\/[\w.-]+$/)) { + return false; + } + return path; +}; + +if (!safePath(info.homedir)) { + return callback(new Error("Unsafe home directory path")); +} +``` + +--- + +## 6. Username Validation Regex Too Restrictive + +**Location**: `lib/user.js:8` + +**Problem**: The username regex rejects valid Linux usernames on some systems: + +```javascript +var validUsernameRegex = /^([a-z_][a-z0-9_-]{0,30})$/; +``` + +Linux allows usernames up to 32 characters (with recent glibc), but this regex: +- Rejects uppercase letters (though unusual, they are allowed in some configurations) +- Rejects usernames starting with numbers +- May reject legitimate edge cases + +**Impact**: Users with valid system accounts may be unable to use this library. + +**Recommendation**: Update regex to match modern Linux standards (POSIX says 1-255 chars, glibc uses 32 max): + +```javascript +var validUsernameRegex = /^([a-z_][a-z0-9_-]{0,30})$/; +// Current: correct for most cases, but could expand to: +var validUsernameRegex = /^([a-z_][a-z0-9_-]{0,30}[a-z0-9_-]?|[a-z_])$/i; +``` + +Or at minimum, document the restriction clearly. + +--- + +## 7. No Process Timeout Handling + +**Location**: `lib/user.js:10-32` (`spawnWrapper`) + +**Problem**: `spawnWrapper` has no mechanism to timeout or kill long-running processes. If a spawned process hangs (e.g., `passwd` prompting for something unexpected), the callback will never fire. + +**Impact**: Applications may hang indefinitely waiting for a callback that never comes. + +**Recommendation**: Add optional timeout support: + +```javascript +function spawnWrapper(command, args, stdin, callback, timeoutMs) { + var timer; + if (timeoutMs) { + timer = setTimeout(function() { + _p.kill(); + callback(new Error("Process timed out"), ""); + }, timeoutMs); + } + // ... existing code ... +} +``` + +--- + +## 8. Module Structure Issue + +**Location**: `lib/promise.js:4` vs `index.js:15` + +**Problem**: When using `require('./lib/promise')` directly, it requires `./user` which triggers the platform check in `index.js` twice: + +```javascript +// lib/promise.js +var lib = require("./user"); // This loads index.js, not lib/user.js directly +``` + +The `require("./user")` in `lib/promise.js` will resolve to `lib/user.js` (via Node's require resolution), but this means: +- The file is `lib/user.js` not `index.js` +- The platform/root checks in `index.js` are bypassed when using promise.js directly + +**Impact**: If someone requires `require('linux-sys-user/lib/promise')` directly, they bypass platform checks. While unlikely, it creates an inconsistent entry point. + +**Recommendation**: This is a minor issue. Consider if direct access to `lib/promise.js` should be supported or blocked. + +--- + +## 9. Missing Error Handling in `addUser` Success Path + +**Location**: `lib/user.js:128-131` + +**Problem**: In `addUser`, if `getUserInfo` fails on line 130, the error is not propagated: + +```javascript +spawnWrapper( + "useradd", + [].concat(args_array, [args.username]), + null, + function (error, data) { + if (error) callback(error); + exports.getUserInfo(args.username, callback); // Error ignored here + } +); +``` + +If `useradd` succeeds but `getUserInfo` fails, the error is silently lost and `callback` may be called twice (once with user info, once with error). + +**Recommendation**: + +```javascript +function (error, data) { + if (error) return callback(error); + exports.getUserInfo(args.username, function(err, user) { + if (err) return callback(err); + callback(null, user); + }); +} +``` + +--- + +## 10. SSH Key Addition Doesn't Respect Existing Keys + +**Location**: `lib/user.js:406-415` + +**Problem**: `addSSHtoUser` appends the key but doesn't check for duplicates: + +```javascript +fs.appendFile( + info.homedir + "/.ssh/authorized_keys", + key, + function (err) { + // Appends regardless of existing entries + } +); +``` + +**Impact**: The same SSH key can be added multiple times to `authorized_keys`. + +**Recommendation**: Check for key existence before appending: + +```javascript +fs.readFile(info.homedir + "/.ssh/authorized_keys", "utf8", function(err, existing) { + if (err && err.code !== 'ENOENT') return callback(err); + var keys = existing ? existing.split('\n') : []; + if (keys.some(function(line) { return line.trim() === key.trim(); })) { + return callback(null, true); // Key already exists + } + fs.appendFile(/* ... */); +}); +``` + +--- + +## Summary Table + +| # | Issue | Severity | Effort | +|---|-------|----------|--------| +| 1 | Inconsistent error handling in spawnWrapper | Medium | Low | +| 2 | TypeScript boolean vs string for selinux_user | Low | Trivial | +| 3 | Fragile date parsing in getExpiration | Medium | Medium | +| 4 | Unreliable setPassword stdin handling | Medium | Medium | +| 5 | Potential path injection in addSSHtoUser | Medium | Low | +| 6 | Username regex too restrictive | Low | Trivial | +| 7 | No process timeout | Low | Low | +| 8 | Module structure issue | Low | N/A | +| 9 | Missing error handling in addUser | Medium | Low | +| 10 | Duplicate SSH keys not checked | Low | Low | diff --git a/issue.md b/issue.md new file mode 100644 index 0000000..774ee45 --- /dev/null +++ b/issue.md @@ -0,0 +1,308 @@ +# Issue Report: linux-sys-user + +This document details issues found during code review of the linux-sys-user codebase. + +--- + +## 1. Inconsistent Error Handling in `spawnWrapper` + +**Location**: `lib/user.js:10-32` + +**Problem**: The `spawnWrapper` function has inconsistent error handling behavior. + +```javascript +_p.on("error", function () { + callback(stderr || true, stdout); // Line 26-27 +}); +_p.on("exit", function () { + callback(stderr, stdout); // Line 29-30 +}); +``` + +When a spawn error occurs (e.g., command not found), it passes `stderr || true` to the callback's error parameter. When the process exits normally, it passes just `stderr`. This means: + +- On spawn error: callback receives truthy error (boolean `true` if stderr is empty) +- On exit with error: callback receives stderr string +- On exit with success: callback receives empty string/falsy + +**Impact**: Calling code cannot reliably distinguish between: +1. Process failed to spawn (command doesn't exist) +2. Process ran but returned an error +3. Process succeeded + +**Recommendation**: Normalize error handling to always pass either an `Error` object or `null`: + +```javascript +_p.on("error", function (err) { + callback(err || new Error("Spawn failed"), stdout); +}); +_p.on("exit", function (code) { + if (code !== 0) { + callback(new Error(stderr || `Process exited with code ${code}`), stdout); + } else { + callback(null, stdout); + } +}); +``` + +--- + +## 2. TypeScript Type Mismatch for `selinux_user` + +**Location**: `lib/user.js:79-81` and `index.d.ts:11` + +**Problem**: The TypeScript definition declares `selinux_user` as `boolean`, but the implementation treats it as a string. + +```typescript +// index.d.ts line 11 +selinux_user?: boolean; + +// lib/user.js lines 79-81 +selinux_user: function (SEUSER) { + if (SEUSER) { + return ["--selinux-user", SEUSER]; + } +}, +``` + +The `useradd` command expects `--selinux-user ` where `` is a SELinux user name (e.g., `staff_u`, `user_u`), not a boolean. + +**Impact**: TypeScript users will receive incorrect type hints and may pass boolean values, which will fail at runtime or produce incorrect `useradd` arguments. + +**Recommendation**: Fix the TypeScript type: + +```typescript +selinux_user?: string; +``` + +--- + +## 3. `setPassword` Stdin Handling May Be Unreliable + +**Location**: `lib/user.js:196-218` + +**Problem**: The `setPassword` function writes to stdin of the `passwd` command: + +```javascript +var _p = spawn("passwd", [username]); +_p.stdin.write(password + "\n"); +_p.stdin.write(password + "\n"); +_p.stdin.end(); +``` + +This approach may fail because: +1. `passwd` may prompt for the current password before asking for a new one (depending on context and security settings) +2. stdin buffering may cause the password to be read after the prompt is displayed +3. Some systems have `pam_chauthtok` requirements that need full interactive password change flow + +**Impact**: Password setting may fail silently or inconsistently across different Linux distributions and configurations. + +**Recommendation**: Consider alternatives: +1. Use `chpasswd` command which accepts `username:password` format via stdin: `echo "user:pass" | chpasswd` +2. Use `openssl` to generate a salted hash and directly edit `/etc/shadow` (requires careful implementation) +3. Document this limitation clearly and note it requires specific system configurations + +--- + +## 4. Fragile Date Parsing in `getExpiration` + +**Location**: `lib/user.js:319-354` + +**Problem**: The `getExpiration` function parses output from `chage --list` assuming a specific format: + +```javascript +data = { + changed: new Date(data[0].split(": ")[1]), + passwordExpires: data[1].split(": ")[1] === "never" ? null : new Date(data[1].split(": ")[1]), + // ... +}; +``` + +This parsing is fragile because: +1. It assumes English output with `: ` delimiter +2. System locale changes could localize date strings causing `new Date()` to fail +3. Different `chage` implementations may format output differently + +**Impact**: On non-English systems or systems with custom locale settings, this function will either throw an error or return invalid Date objects. + +**Recommendation**: Either: +1. Document this as a known limitation requiring English locale +2. Use regex patterns that extract the value between `: ` and the newline, then use a locale-aware date parser +3. Parse `/etc/shadow` directly for more reliable data extraction + +--- + +## 5. Potential Path Injection in `addSSHtoUser` + +**Location**: `lib/user.js:377-419` + +**Problem**: The `addSSHtoUser` function uses `exec` with string formatting for shell commands: + +```javascript +var commands = [ + util.format("mkdir %s/.ssh;", info.homedir), + util.format("touch %s/.ssh/authorized_keys;", info.homedir), + util.format("chown 700 %s/.ssh;", info.homedir), + // ... +].join(" "); +exec(commands, function (err, stdout, stderr) { ... }); +``` + +While `info.homedir` comes from `/etc/passwd` parsing (trusted on a properly secured system), if an attacker could modify `/etc/passwd` or if there's a misconfiguration, the homedir could contain shell metacharacters. + +**Impact**: Potential for command injection if homedir contains characters like `;`, `|`, `&`, etc. + +**Recommendation**: Validate homedir path before use: + +```javascript +if (!/^\/[a-zA-Z0-9_\/-]+$/.test(info.homedir)) { + return callback(new Error("Invalid homedir path")); +} +``` + +Or use spawn with argument arrays instead of shell execution. + +--- + +## 6. Commented-Out Code in `setPassword` + +**Location**: `lib/user.js:206-208` + +**Problem**: There is commented-out code suggesting a previous implementation approach: + +```javascript +// spawnWrapper('password', [username], password+'\n'+password+'\n', function(error, data){ +// console.log(error, data) +// }) +``` + +**Impact**: Dead code creates confusion about the intended implementation. This should either be removed or the approach should be documented. + +**Recommendation**: Remove the commented code or document why it was abandoned. + +--- + +## 7. `addUser` Always Calls `getUserInfo` on Error + +**Location**: `lib/user.js:128-131` + +**Problem**: In the `addUser` function: + +```javascript +spawnWrapper("useradd", [].concat(args_array, [args.username]), null, + function (error, data) { + if (error) callback(error); + exports.getUserInfo(args.username, callback); // Called even when error occurred! + } +); +``` + +When `useradd` fails, the error is passed to callback, but then `getUserInfo` is still called (and overwrites the error). If `useradd` fails, `getUserInfo` will return `null` as the user doesn't exist, and the actual error from `useradd` is lost. + +**Impact**: Error messages from `useradd` failures are lost or overwritten. + +**Recommendation**: Return early on error: + +```javascript +if (error) return callback(error); +exports.getUserInfo(args.username, callback); +``` + +--- + +## 8. Same Issue in `addGroup` + +**Location**: `lib/user.js:226-228` + +**Problem**: Same pattern as issue #7: + +```javascript +spawnWrapper("groupadd", [groupname], null, function () { + exports.getGroupInfo(groupname, callback); // No error handling! +}); +``` + +Errors from `groupadd` are silently ignored. The callback receives whatever `getGroupInfo` returns. + +**Impact**: Group creation failures are not reported to the caller. + +**Recommendation**: Add error handling: + +```javascript +spawnWrapper("groupadd", [groupname], null, function (error) { + if (error) return callback(error); + exports.getGroupInfo(groupname, callback); +}); +``` + +--- + +## 9. No Timeout Handling + +**Location**: All spawn-based functions + +**Problem**: None of the `spawn` or `exec` calls implement timeouts. A hung process (e.g., due to system configuration issues, LDAP/SSSD delays, etc.) could cause the library to hang indefinitely. + +**Impact**: Applications using this library may hang waiting for child processes that will never complete. + +**Recommendation**: Add a timeout mechanism: + +```javascript +function spawnWithTimeout(command, args, stdin, timeoutMs, callback) { + var timer; + var p = spawn(command, args); + + if (timeoutMs) { + timer = setTimeout(function() { + p.kill(); + callback(new Error("Process timed out")); + }, timeoutMs); + } + + // ... rest of spawnWrapper logic + _p.on("exit", function() { + if (timer) clearTimeout(timer); + callback(stderr, stdout); + }); +} +``` + +--- + +## 10. Documentation vs Implementation: `addUser` String Argument + +**Location**: `lib/user.js:109-116` and `README.md` + +**Problem**: When `addUser` receives a string, it converts it to an object: + +```javascript +if (typeof args === "string") { + args = { + username: args, + create_home: true, + }; +} +``` + +This is documented in the README (line 51 shows passing an object), but the string shortcut is a convenience that isn't clearly documented. + +**Impact**: Users may not know they can pass just a username string. + +**Recommendation**: Either document this shortcut or remove it for API clarity. + +--- + +## Summary Table + +| # | Issue | Severity | Location | +|---|-------|----------|----------| +| 1 | Inconsistent error handling in spawnWrapper | Medium | lib/user.js:26-30 | +| 2 | TypeScript type mismatch for selinux_user | Low | index.d.ts:11, lib/user.js:79-81 | +| 3 | setPassword stdin handling unreliable | Medium | lib/user.js:196-218 | +| 4 | Fragile date parsing in getExpiration | Medium | lib/user.js:319-354 | +| 5 | Potential path injection in addSSHtoUser | Medium | lib/user.js:387-398 | +| 6 | Commented-out dead code | Low | lib/user.js:206-208 | +| 7 | addUser loses error on failure | Medium | lib/user.js:128-131 | +| 8 | addGroup ignores errors | Medium | lib/user.js:226-228 | +| 9 | No timeout handling | Low | All spawn/exec calls | +| 10 | Undocumented string argument shortcut | Low | lib/user.js:109-116 | diff --git a/package.json b/package.json index f5fadbd..8c05dd3 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "linux-sys-user", - "version": "2.0.0", + "version": "2.0.1", "description": "Node.js module for Linux user and group management with zero dependencies", "main": "index.js", "types": "index.d.ts",