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

v1.6.0: pam_env test always crashes in pam_open_session() #738

Closed
thkukuk opened this issue Jan 18, 2024 · 19 comments
Closed

v1.6.0: pam_env test always crashes in pam_open_session() #738

thkukuk opened this issue Jan 18, 2024 · 19 comments
Labels

Comments

@thkukuk
Copy link
Contributor

thkukuk commented Jan 18, 2024

Regression compared to 1.5.3.
We see on all builds that make check crashes in tst-pam_env-retval:

[  131s] make[4]: Entering directory '/home/abuild/rpmbuild/BUILD/Linux-PAM-1.6.0/modules/pam_env'
[  131s] ../../build-aux/test-driver: line 112: 15021 Aborted                 (core dumped) "$@" >> "$log_file" 2>&1
[  131s] FAIL: tst-pam_env-retval
...
[  131s] tst-pam_env-retval.c:187: Assertion failed: PAM_PERM_DENIED (0x6) == pam_open_session(pamh, 0) (0)
[  131s] FAIL tst-pam_env-retval (exit status: 134)

It is always in pam_open_session(), but not always the same call to it. strace shows a wired mix of loading modules, libraries and config files from the running system and from the build system.

@ldv-alt ldv-alt added the bug label Jan 18, 2024
@ldv-alt
Copy link
Member

ldv-alt commented Jan 18, 2024

There shouldn't be any pam modules or pam config files loaded from the running system.

@ldv-alt
Copy link
Member

ldv-alt commented Jan 18, 2024

Could it be something related to --enable-econf configuration? We don't test this in CI yet because it needs libeconf >= 0.5.0.

@stoeckmann
Copy link
Contributor

It's my first assumption as well. The pam_env module is the first econf module covered with a test. pam_shells uses econf, but has no test.

@thkukuk
Copy link
Contributor Author

thkukuk commented Jan 18, 2024

That was our assumption first, too, but disabling libeconf did not change the picture.

@stoeckmann
Copy link
Contributor

Does it happen with other tests as well? I mean, did you try to disable the test and let others run, too?

@ldv-alt
Copy link
Member

ldv-alt commented Jan 18, 2024

Also, tst-pam_env-retval.c was already present in v1.5.3.

@thkukuk
Copy link
Contributor Author

thkukuk commented Jan 18, 2024

In v1.5.3 it worked
Didn't had yet the time to diff 1.5.3 with 1.6.0, tomorrow...

@ldv-alt
Copy link
Member

ldv-alt commented Jan 18, 2024

Tried to reproduce locally,

strace -e%file -o'|grep env' .libs/lt-tst-pam_env-retval

doesn't show any attempts to access system pam files.

@stoeckmann
Copy link
Contributor

Can reproduce it. Not sure if this is how it happened, though:

./configure --enable-vendordir=/tmp
make
install -D modules/pam_env/pam_env.conf /tmp/security/pam_env.conf
make check

Since VENDORDIR is set, the supposedly non-existing file is parsed. This leads to a successful pam_open_session which in turn makes the test fail.

@thkukuk
Copy link
Contributor Author

thkukuk commented Jan 18, 2024

Copying lt-tst-pam_env-retval.c from 1.5.3 to 1.6.0 let the test suite pass.

Ok, to make the thing complicated:

  • on openSUSE Tumbleweed it crashes with and without libeconf
  • on SLES15 SP5 it crashes only with libeconf, not without.

So there are two bugs, one with libeconf support in pam_env and a second one with current toolchain?

@thkukuk
Copy link
Contributor Author

thkukuk commented Jan 18, 2024

Since VENDORDIR is set, the supposedly non-existing file is parsed. This leads to a successful pam_open_session which in turn makes the test fail.

This fit's with my strace, that files from the system and the build directory are mixed up.

@stoeckmann
Copy link
Contributor

It cannot be an explanation though, because 1.5.3 fails with my instructions as well...

@ldv-alt
Copy link
Member

ldv-alt commented Jan 18, 2024

I don't have a reproducer yet, but I think I can fix the !USE_ECONF case.

ldv-alt added a commit that referenced this issue Jan 18, 2024
* modules/pam_env/pam_env.c (_parse_config_file) [!USE_ECONF &&
VENDOR_DEFAULT_CONF_FILE]: Do not fallback to vendor pam_env.conf file
if the config file is specified via module arguments.

Link: #738
Fixes: v1.5.3~69 ("pam_env: Use vendor specific pam_env.conf and environment as fallback")
ldv-alt added a commit that referenced this issue Jan 18, 2024
* modules/pam_env/pam_env.c (_parse_config_file) [!USE_ECONF &&
VENDOR_DEFAULT_CONF_FILE]: Do not fallback to vendor pam_env.conf file
if the config file is specified via module arguments.

Link: #738
Fixes: v1.5.3~69 ("pam_env: Use vendor specific pam_env.conf and environment as fallback")
@stoeckmann
Copy link
Contributor

stoeckmann commented Jan 18, 2024

Finally. My build fails now with a clean econf build, i.e. no additional configuration options. Time to investigate.

It's a current Linux From Scratch amd64 system with libeconf master on github.

Using the test from 1.5.3 fixes it.

@stoeckmann
Copy link
Contributor

Using the test from 1.5.3 fixes it.

The econf code does not remove escaped newlines. We have added a test for this in 1.6.0.
The whole string looks like:

NAME DEFAULT=@{PAM_\
USER}\\name

instead of the expected

NAME DEFAULT=@{PAM_USER}\\name

@stoeckmann
Copy link
Contributor

stoeckmann commented Jan 18, 2024

The PR #741 removes the escaped newlines the way we do without econf. Lets my build finish successfully.

This in turn means that escaped newlines properly work now as well. Hopefully.

@thkukuk can you please test?

@ldv-alt
Copy link
Member

ldv-alt commented Jan 18, 2024

In other words, since the introduction of libeconf support in pam_env by commit 6135c45 pam_env parses its config files differently depending on whether libeconf support is enabled, but we noticed this only when the test for this difference was added in v1.6.0.

@stoeckmann
Copy link
Contributor

Yes. I don't know if our previous fixes and my latest PR will fix ALL cases, but this one is definitely based on different behaviours between the libeconf line parser and our parser.

@thkukuk
Copy link
Contributor Author

thkukuk commented Jan 19, 2024

I added now PR#739, PR#740 and PR#741 to our package and it passes now all tests.
Thanks for the fixes!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants