Config file support #326

dacav · 2024-12-13T15:08:07Z

Fixes #265

dacav · 2024-12-13T15:10:07Z

Manpages update

cfg.c

README

man/pam_u2f.8.txt

LDVG

This looks like a pretty good start to me. I've left mostly nitpicking comments in-line. 🙂

Makefile.am

README

man/pam_u2f.8.txt

tests/cfg.c

dacav · 2024-12-18T20:58:15Z

Current situation:

What is left to do for the current PR (soon follow-up):

A proper MAX_PATH (currently using hard-wired 4096 - Linux).
A temporary --with-security-confdir solution, mimicking --with-pam-dir (future refinement)
Trailing comments and spaces around '='
Only file owner can write (hopefully that's root)
Only root can own the file (requires fakeroot-ish behaviour)
Comprehensive config unit test
Inject the security-confdir into manpages (currently "/etc/security" is a fixed string

dacav · 2025-01-02T11:35:22Z

Oh no. I figured out this even more work!

Autoconf BUG (?)

./configure --with-sconf-dir=/a
make && sudo make install
./configure --with-sconf-dir=/b
make # WILL NOT RECOMPILE (well, only manpage)
sudo make install # not what expected: old binary is installed again

This is a little pathological (maybe low prio?) but actually a defect.

Vulnerable for untidy sysadmin:

Currently my code is checking that the configuration file (1) belongs to root, (2) can only be written by root, (3) is not a symlink. But I'm not checking the status of the above directories... Therefore, assuming that pam_u2f.so is configured to use /my/hopefully/notalink/pam_u2f.conf as a configuration file, in the (also pathological?) situation the unprivileged user might be able to hijack the configuration:

  user@localhost:/my$ ls -lR .
  .:
  total 4
  drwxrwxrwx 4 root root 4096 Jan  2 11:23 hopefully [1]

  ./hopefully:
  total 8
  drwxr-xr-x 2 root root 4096 Jan  2 11:15 notalink
  drwxr-xr-x 2 root root 4096 Jan  2 11:23 oops

  ./hopefully/notalink:
  total 4
  -rw-r--r-- 1 root root 13 Jan  2 11:05 pam_u2f.conf [2]

  ./hopefully/oops:
  total 4
  -rw-r--r-- 1 root root 13 Jan  2 11:23 pam_u2f.conf [2]

[1] world-writable directory
[2] a root-owned-not-writable copy exists

Attack:

  user@localhost:/my$ cd hopefully
  user@localhost:/my/hopefully$ mv notalink/ goodbye
  user@localhost:/my/hopefully$ ln -s oops notalink
  user@localhost:/my/hopefully$ cd ..
  user@localhost:/my$ ls -lR
  .:
  total 4
  drwxrwxrwx 4 root root 4096 Jan  2 11:25 hopefully

  ./hopefully:
  total 8
  drwxr-xr-x 2 root root 4096 Jan  2 11:15 goodbye
  lrwxrwxrwx 1 user user    4 Jan  2 11:25 notalink -> oops
  drwxr-xr-x 2 root root 4096 Jan  2 11:23 oops

  ./hopefully/goodbye:
  total 4
  -rw-r--r-- 1 root root 13 Jan  2 11:05 pam_u2f.conf

  ./hopefully/oops:
  total 4
  -rw-r--r-- 1 root root 15 Jan  2 11:23 pam_u2f.conf

Having it into another module will prevent the code from being messy later. The parsing procedure is taken verbatim: no semantic change, no behavioural change.

The configuration file defines the default behaviour of pam_u2f. Individual module invocations under /etc/pam.d can override settings. The file-system location of the config file is by default $sysconfdir/security/pam_u2f.conf, where $sysconfdir is supplied at build time. A new module configuration, "conf=", allows to override it at runtime. Only absolute paths are accepted.

- split-input format: add trailing blob for config file The corpus needs some update. - wrappers (-Wl,--wrap) integrate fuzzing of the configuration file. The configuration file, mutated by the fuzzer, is made available to the cfg.c implementation. The mock-up works under the assumption that only the cfg.c module works by opening "/" with open(3), and follows up with an alternation of openat(3) and fstat(3) calls.

Generate pam_u2f.8.txt from pam_u2f.8.txt.in, replacing SCONFDIR

dacav · 2025-01-15T07:57:26Z

Rebased on current main.

LDVG

last iteration of nit picking, this looks good to go otherwise. nice work! :)

LDVG · 2025-01-15T09:54:43Z

cfg.c

+//
+// On success returns PAM_SUCCESS
+// On failure returns PAM_SERVICE_ERR and sets errno to indicate the error.
+static int open_safely(int *outfd, size_t *outsize, const char *path) {


I'm not a huge fan of using errno this way. It appears we only need to be able to discern ENOENT, success, and a generic error for everything else. What about returning -ENOENT, a non-negative integer (fd), and -EINVAL respectively instead of the PAM_* value?

The idea behind this is to use PAM_* error codes consistently everywhere for error handling.
It is feasible to do as you suggest, but that would be the only function that does error checking differently.
If that is in your opinion more beautiful than writing errno, then I can comply with your request. Please let me know :-)

I believe we have several other functions that return 0 or 1 or some other error code, I'd rather use "internal" return values for these internal functions, we only really need to concern ourselves with PAM_ return values for functions that set retval in pam_sm_authenticate()

LDVG · 2025-01-15T09:55:52Z

cfg.c

+  len = strlen(path);
+  if (!len || path[0] != '/' || path[len - 1] == '/') {
+    errno = EINVAL;
+    return PAM_SERVICE_ERR;
+  }


Wouldn't *path != '/' suffice? S_ISREG should catch trying to open up a directory.

Correct.
A left-over from previous iteration (path security) where I used strtok_r to split string on /.
On such situation this was a corner case to check.

LDVG · 2025-01-15T09:56:33Z

cfg.c

+  if (fd == -1)
+    return PAM_SERVICE_ERR;
+
+  if (fstat(fd, &st))


I'd prefer to be explicit here with fstat(fd, &st) != 0.

LDVG · 2025-01-15T09:59:05Z

cfg.c

+    if (strncmp(argv[i], "conf=", strlen("conf=")) == 0)
+      continue;
+


Since cfg_load_arg() does not handle conf=, should we just drop this short circuiting?

LDVG · 2025-01-15T10:02:18Z

cfg.h

+#include <stdio.h>
+
+#define CFG_DEFAULT_PATH (SCONFDIR "/pam_u2f.conf")
+#define CFG_MAX_FILE_SIZE 4096 // Arbitrary


I'd drop the comment

When I see a constant, my first thought is "why not double or half that amount"?
Anyway... OK. 😁 no strong opinion.

No strong opinions from my side either, I just don't get that much more information from this comment :)

LDVG · 2025-01-15T10:22:50Z

README

+A configuration file can be used to set the default
+<<moduleArguments,module arguments>>.
+
+The file has a `name = value` format, with comments starting with the `#`


This section reads like a list, but is not a list, should we rephrase or add bullet points?

(same comment for man pages. as an additional follow up, we might want to consider a pam_u2f.conf.5)

dacav requested a review from LDVG December 13, 2024 15:08

dacav self-assigned this Dec 13, 2024

dacav marked this pull request as draft December 13, 2024 15:08

dacav force-pushed the dacav/config_file branch 2 times, most recently from 611e678 to 257c433 Compare December 16, 2024 23:23

dacav commented Dec 16, 2024

View reviewed changes

cfg.c Outdated Show resolved Hide resolved

dacav commented Dec 16, 2024

View reviewed changes

cfg.c Outdated Show resolved Hide resolved

dacav commented Dec 16, 2024

View reviewed changes

README Outdated Show resolved Hide resolved

dacav commented Dec 16, 2024

View reviewed changes

man/pam_u2f.8.txt Outdated Show resolved Hide resolved

dacav force-pushed the dacav/config_file branch 2 times, most recently from 81fb36f to e9d2621 Compare December 17, 2024 08:24

dacav marked this pull request as ready for review December 17, 2024 08:36

dacav force-pushed the dacav/config_file branch 2 times, most recently from 3b245e0 to 5365c0d Compare December 17, 2024 08:50

LDVG reviewed Dec 17, 2024

View reviewed changes

dacav marked this pull request as draft December 17, 2024 14:45

dacav force-pushed the dacav/config_file branch from 5365c0d to 98fdb4d Compare December 18, 2024 20:57

dacav force-pushed the dacav/config_file branch 6 times, most recently from 0adeda2 to a6e93f3 Compare December 23, 2024 22:50

dacav marked this pull request as ready for review January 2, 2025 08:51

dacav force-pushed the dacav/config_file branch 2 times, most recently from 36e1255 to 58e0177 Compare January 8, 2025 12:45

dacav marked this pull request as draft January 8, 2025 12:59

dacav force-pushed the dacav/config_file branch 3 times, most recently from 9d2c5d1 to f90d66a Compare January 13, 2025 08:43

dacav marked this pull request as ready for review January 13, 2025 08:44

dacav force-pushed the dacav/config_file branch from f90d66a to 3a0ebb6 Compare January 13, 2025 15:02

dacav added 6 commits January 15, 2025 08:56

pam: Introduce cfg module

4216abc

Having it into another module will prevent the code from being messy later. The parsing procedure is taken verbatim: no semantic change, no behavioural change.

tests: Add unit tests for conf file

dabe69e

README: update with info about conf file

727d605

man: update with info about conf file

6faab0d

Generate pam_u2f.8.txt from pam_u2f.8.txt.in, replacing SCONFDIR

dacav force-pushed the dacav/config_file branch from 3a0ebb6 to 6faab0d Compare January 15, 2025 07:57

LDVG reviewed Jan 15, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Config file support #326

Config file support #326

dacav commented Dec 13, 2024

dacav commented Dec 13, 2024 •

edited

Loading

LDVG left a comment

dacav commented Dec 18, 2024 •

edited

Loading

dacav commented Jan 2, 2025

dacav commented Jan 15, 2025

LDVG left a comment

LDVG Jan 15, 2025

dacav Jan 15, 2025

LDVG Jan 15, 2025

LDVG Jan 15, 2025

dacav Jan 15, 2025

LDVG Jan 15, 2025

LDVG Jan 15, 2025

LDVG Jan 15, 2025

dacav Jan 15, 2025

LDVG Jan 15, 2025

LDVG Jan 15, 2025

		if (strncmp(argv[i], "conf=", strlen("conf=")) == 0)
		continue;

Config file support #326

Are you sure you want to change the base?

Config file support #326

Conversation

dacav commented Dec 13, 2024

dacav commented Dec 13, 2024 • edited Loading

LDVG left a comment

Choose a reason for hiding this comment

dacav commented Dec 18, 2024 • edited Loading

Current situation:

What is left to do for the current PR (soon follow-up):

dacav commented Jan 2, 2025

Autoconf BUG (?)

Vulnerable for untidy sysadmin:

dacav commented Jan 15, 2025

LDVG 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dacav commented Dec 13, 2024 •

edited

Loading

dacav commented Dec 18, 2024 •

edited

Loading