Fixes for v2.5.4: crash on fork+exec (with tests for script-running), printf specifier, pass correct stripc names - #638
Conversation
|
Hello, this PR is similar to PR#637 in that that it fixes the
|
The debug message in eaptls_send() uses "%d ... %.*B", which consumes three arguments: the byte count for %d, then an int precision and a buffer pointer for %.*B. Only two were passed, so the count was taken as the %d, the dummy buffer pointer was taken as the precision, and vslprintf() fetched the %B data pointer from whatever followed on the argument list. With "debug" enabled this reads through an undefined pointer for an effectively arbitrary number of bytes and can crash pppd during an EAP-TLS handshake. Pass res for %d and MIN(res, 20) as the precision, as intended. Fixes: dd5acd9 ("pppd/EAP-TLS: Send zero byte as protected success indication with TLS 1.3") Signed-off-by: Adam Zegarek <keragez@gmail.com>
…up and auth-down run_program() exports its name argument to the script as PPP_SCRIPT_INSTANCE. pppd.8 documents it as "the name of the intended script, as documented, not as referenced", and recommends it to scripts because with strict-script-checks (the default) a #! script is run via fexecve() and its $0 no longer identifies which hook it is. Three call sites pass a name that does not match the script they run: - link_down() runs path_auth_down labelled "auth-up". This is the normal link-down path whenever the peer authenticated. - ipv6cp_up() runs path_ipv6up labelled "ipv6-ip". This is every time IPv6CP comes up. - ipcp_script_done() runs path_ipup labelled "ip-down" when the link came back up while ip-down was still running. The correct script file was always executed; only the environment variable was wrong. A single script installed for several hooks that dispatches on $PPP_SCRIPT_INSTANCE would take the wrong branch, e.g. re-adding rules on auth-down, or doing nothing on ipv6-up. Fixes: e87ddf0 ("run_program: Export the name of the script via PPP_SCRIPT_INSTANCE") Signed-off-by: Adam Zegarek <keragez@gmail.com>
The forked child closes the TDB with tdb_close(), which zeroes and frees the context, but leaves the global pppdb pointing at the freed memory. This was harmless until e87ddf0, because nothing in the child touched pppdb before execve(). run_program() now calls ppp_script_setenv("PPP_SCRIPT_INSTANCE", ...) in the child, which sees pppdb != NULL and calls update_db_entry() -> tdb_store() on the freed context. The allocation of the new environment string can reuse that chunk, so the result depends on heap layout: often the child survives, sometimes it dies with SIGSEGV before it executes the script. The parent only logs "Child process ... terminated with signal 11" and carries on, so ip-up, ip-down, auth-up, auth-down etc. are silently skipped. Seen in the field on 2.5.4 with PPPoE as SIGSEGV core dumps of pppd (the script child), and reproduced on x86_64 with the new script-run test, where the auth-down child crashed. Only builds with --enable-multilink (PPP_WITH_TDB) are affected. Fixes: e87ddf0 ("run_program: Export the name of the script via PPP_SCRIPT_INSTANCE") Signed-off-by: Adam Zegarek <keragez@gmail.com>
jkroonza
left a comment
jkroonza
left a comment
There was a problem hiding this comment.
I'm probably missing something obvious, but please confirm if there is a reason to skip the run_program() tests with TDB disabled - surely the scripts should work with it enabled as well as disabled?
I know the regression was only with it enabled, but we can test both ways right?
Otherwise looks all good for me.
The reason was: to have a test in place to look for regression regrading the So, if you haven't build ppp-project with
Well... You're right. It would still check script-running and having correct names in script-calling. Changed skipping to execution. Non-TDB build:-AZ |
|
@jkroonza: So, we're going forward with this patch? |
|
@keragez yes. I'm not sure what the time-lines are that @paulusmack is aiming for a potential follow-up release, I'll include on Gentoo side so long as this is going to potentially bite hard. Would it be useful to introduce a tdb USE flag in your opinion? |
|
Oh, did the ci# test discovered yet another regression? The tests we just enabled helped discover it. I'll look into that.
What's the benefit in that?
-AZ |
|
2.5.4 with TDB there are cases where the scripts won't execute. This is added to the test suite now. The cause was the TDB dangling pointer in the child, which was referenced - if this pointer referenced invalid data we potentially just got the lock error, if it was corrupt in a different way we could crash (and then the scripts would not execute). Let me get the patch into gentoo as -r1. Then I'll look into the USE flag (and yes, I think multilink is more appropriate). Either way - looks like ppp uses a bundled, potentially outdated tdb. @paulusmack would it be possible to enable using the system-installed tdb? |
As to the OmniOS CI failureThe OmniOS failure is not caused by this PR. The new test surfaced an existing 2.5.4 problem that no test had covered before. What happenedAfter the change to also run
Likely causeSince 6a4944f ("pppd: relax and simplify permission check", first released in 2.5.4), I don't have the exact errno: pppd logs Solaris 11.4 passes only because it has no kernel PPP, so all link tests skip there. Linux CI is unaffected. I also don't have an OmniOS machine to fix and test it on locally. What I changed here
@jkroonza : This would be a separate issue for the illumos exec problem. Fixing it means choosing how |
|
Please raise a separate PR/issue for that. You can point to this PR for the tests. @paulusmack may I request this get merged sooner rather than later please. I think a 2.5.5 may be in order once this, #635 and a fix for the OmniOS issues are merged. I'll see if I can bring an OmniOS up somewhere on a VM. Need to bring something up for another purpose anyway. I am going to recommend forking a 2.5 branch at this point with for further fixes pertaining to 2.5.X, with any new features only going to master, targeting that for 2.6.0 eventually some time 2027q1 probably. Things like radius-ng will then also be targeted at that rather than bringing into 2.5.X which I then highly recommend gets a "security & bug fix only" status associated. I think for a 2.6 we should try and focus strongly on increasing test coverage as much as possible. |
Include fairly critical fixes: ppp-project/ppp#638 Signed-off-by: Jaco Kroon <jkroon@gentoo.org>
|
Two things @keragez
|
No existing test installs a hook script, and run_program() returns
before forking when the script file does not exist, so the code between
fork() and execve() was never run by the suite. That is how a SIGSEGV
in the script child (see "pppd: Clear pppdb after tdb_close() in
ppp_safe_fork()") went unnoticed.
pppfns.py: PppPeer gains a scripts={name: text} argument. The scripts
are staged exactly like pap-secrets/chap-secrets: on Linux they are
copied into the root-owned tmpfs behind the bind-mounted confdir and
symlinked from it, elsewhere they are copied into the real confdir
(PPPD_TEST_GLOBAL_CONF=1 hosts only) and removed afterwards. They are
installed mode 755 so ppp_check_access(PPP_FT_EXEC) accepts them.
script-run_test.py installs hooks that append their name,
$PPP_SCRIPT_INSTANCE and arguments to a marker file, then:
- brings a link up and down and checks ip-pre-up (run with wait=1),
ip-up and ip-down (wait=0) all ran;
- repeats with side a as a PAP server and checks auth-up and
auth-down ran;
- checks PPP_SCRIPT_INSTANCE matches each hook (skipped when unset,
so --pppd-bin2 against pre-2.5.4 pppd still works);
- fails if pppd logged a script child terminated by a signal.
The pppdb crash can only happen in builds with TDB
(--enable-multilink), but the test runs on every build: the hooks must
run and be labelled correctly regardless, and the PPP_SCRIPT_INSTANCE
mislabelling fixed in "pppd: Pass correct PPP_SCRIPT_INSTANCE name for
deferred ip-up, ipv6-up and auth-down" affected every build. Without
TDB it prints a note, so that a pass there is not mistaken for coverage
of the pppdb fix. Running everywhere also covers Solaris/illumos, where
multilink is not supported: illumos has no O_PATH, so run_program()
fexecve()s a descriptor opened with O_EXEC, a path the Linux CI jobs
never take.
On OmniOS every hook currently exits with status 0x63 (99, the child's
exit after a failed exec): since 2.5.4, under the default
strict-script-checks, run_program() fails to fexecve() #! scripts from
an O_EXEC descriptor there, while 2.5.3 and earlier exec'd by path.
That is a separate issue, not what this test guards, so on SunOS a hook
that did not run *and* was logged exiting 0x63 is reported as XFAIL.
Every other failure, including a script child killed by a signal, still
fails; if hooks start working on illumos the test just passes.
The test is skipped on non-Linux hosts that already have one of the
hook scripts installed. The marker file is created by the test user
beforehand, since the script child runs with umask 077.
Results on x86_64 Linux:
without TDB: PASS (with note)
TDB, without pppdb fix: FAIL (auth-down child terminated with signal 11)
TDB, with pppdb fix: PASS
Signed-off-by: Adam Zegarek <keragez@gmail.com>
|
Ad.1: I have Opus 5.5 and used it. I employed it mainly for the things these machines are good for - for reading looooong inputs and summarizing. We had a regression from v2.5.2 to v2.5.4, so I looked up the diff myself, and then fed it the diff to look for potential issues. It had failed to find TDB issue first, but found the Tests - mainly AI driven, but read through and tested by me before submitting. I also fed it your Submitting-patches.md policy, and it added some verbiage to commit messages, to make them pass the policy of being "self sufficient without Github's infrastructure". Ad. 2: done, squash pushed, all tests in one commit. |
|
Thanks @keragez, this is great. :) |
That's a good idea. Alternatively we could change to sqlite or similar. |
Let's not create too many options. I think we should also keep backwards compatibility in mind. Consider the use case where I upgrade ppp but don't want to reconnect all dial-in users just to upgrade the database. In this case it's better to just stick everything with TDB. Let's log a separate issue for this. |
3 participants
Situation
When running pppd v2.5.4 on an ppc64 system I encountered a crash of the pppd.
Crash reproduces also on
x86-64bit.Crash after fork
The crash occured in the forked process in the pppd/main.c file around the
ppp_script_setenv("PPP_SCRIPT_INSTANCE", name, 0);call. The execution never reached theexecve(...), whereby the forked pppd instance was never superseeded by the script it intended to run.Analysis of core suggested that the
pppdbwas closed two times. Althoughint tdb_close(TDB_CONTEXT *tdb)usesSAFE_FREEfor the actual freeing, it unconditionally dereferencestdb->map_ptr, so thepppdbptr must beNULLed after freeing.Added test for the fix, to prevent further regressions.
The tests:
Non-TDB build:
TDB build, no pppdb fix:
TDB build, with pppdb fix:
Other issues
Analisys of differences from v2.5.2 also shown:
.*Bin a dbglog function call.-BR,
AZ