Bug 280418 - calendar -a trashes parent's pgrp login setting
Summary: calendar -a trashes parent's pgrp login setting
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: bin (show other bugs)
Version: 14.1-RELEASE
Hardware: Any Any
: --- Affects Some People
Assignee: Kyle Evans
URL: https://reviews.freebsd.org/D46095
Keywords:
Depends on:
Blocks:
 
Reported: 2024-07-23 18:10 UTC by Steve Watt
Modified: 2024-09-09 01:41 UTC (History)
5 users (show)

See Also:
kevans: mfc-stable14?
kevans: mfc-stable13?
kevans: needs_errata? (secteam)


Attachments
Patch for src/usr.bin/calendar/calendar.c to add the missing setsid() (565 bytes, patch)
2024-07-23 18:10 UTC, Steve Watt
no flags Details | Diff
Proposed errata notice (4.29 KB, text/plain)
2024-08-16 22:07 UTC, Kyle Evans
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Steve Watt 2024-07-23 18:10:21 UTC
Created attachment 252245 [details]
Patch for src/usr.bin/calendar/calendar.c to add the missing setsid()

The presenting symptom is that the daily and security emails from cron/periodic were arriving with the envelope-sender set to some completely random user.

After much one-experiment-per-day debugging, I isolated it down to 300.calendar changing the login userid. It seems to be missing its setsid() before calling setusercontext().

Easy-to-demonstrate sample:
> (root@rivendell) 43# sh
> # id -p
> uid     root
> groups  users wheel www vboxusers
> # calendar -a
> # id -p
> login   _tss
> uid     root
> groups  users wheel www vboxusers
> # exit
> (root@rivendell) 44# id -p
> login   _tss
> uid     root
> (root@rivendell) 45#

The fix is startlingly simple, with a little help on -questions from Kyle Evans, see attached.
Comment 1 Kyle Evans freebsd_committer freebsd_triage 2024-07-24 04:30:33 UTC
I went for a slightly different fix that entails less overhead per-user, see https://reviews.freebsd.org/D46095.
Comment 2 Steve Watt 2024-07-24 06:36:34 UTC
(In reply to Kyle Evans from comment #1)
Will that make the envelope sender for the email that calendar generates come out correctly?
Comment 3 Kyle Evans freebsd_committer freebsd_triage 2024-07-24 15:59:57 UTC
(In reply to Steve Watt from comment #2)

calendar(1) constructs mail headers based on the pwent we forked for; unless mail(1) does something funky, it should be fine.
Comment 4 commit-hook freebsd_committer freebsd_triage 2024-08-05 18:44:11 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=6cb8b61efe8899ee9194563108d0ae90c1eb89e3

commit 6cb8b61efe8899ee9194563108d0ae90c1eb89e3
Author:     Kyle Evans <kevans@FreeBSD.org>
AuthorDate: 2024-08-05 18:43:56 +0000
Commit:     Kyle Evans <kevans@FreeBSD.org>
CommitDate: 2024-08-05 18:43:56 +0000

    calendar: don't setlogin(2) in the -a user handlers

    As of e67975d331 ("Fix 'calendar -a' in several ways."), `calendar -a`
    will now fork off a new process for each user and do all of its own
    processing in the user's own context.

    As a side-effect, calendar(1) started calling setlogin(2) in each of the
    forked processes and inadvertently hijacked the login name for the
    session it was running under, which was typically not a fresh session
    but rather that of whatever cron/periodic run spawned it.  Thus, daily
    and security e-mails started coming from completely arbitrary user.

    We could create a new session, but it appears that nothing calendar(1)
    does really needs the login name to be clobbered; opt to just avoid the
    setlogin(2) call entirely rather than incur the overhead of a new
    session for each process.

    PR:             280418
    Reviewed by:    des, olce
    Fixes:          e67975d331 ("Fix 'calendar -a' in several ways.")
    Differential Revision:  https://reviews.freebsd.org/D46095

 usr.bin/calendar/calendar.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Comment 5 Kyle Evans freebsd_committer freebsd_triage 2024-08-05 18:53:11 UTC
I don't have a strong opinion on whether we should EN this or not- I could see some harm in the deceptive/unexpected e-mail sender, but I also don't think we've really received much in the way of complaints from this.   Let's ask secteam@ to weigh in, maybe.
Comment 6 Steve Watt 2024-08-05 18:56:48 UTC
The first (very minor) security exposure I can think of is that usernames and fullnames are exposed to whoever is receiving the periodic/daily and periodic/security email. However, if there are other mail-generating scripts in periodic/local that sends to someone other than an administrator, that could leak usernames/fullnames.
Comment 7 Kyle Evans freebsd_committer freebsd_triage 2024-08-05 19:37:48 UTC
(In reply to Steve Watt from comment #6)

Note that secteam handles all maintenance to a releng branch once the release is out, so they by necessity have to evaluate for both security exposure as well as general functional concerns.  e.g., if there's a larger concern here that some filtering didn't apply at the target inbox because the sender was different than one might have expected, that's also something they have to take into consideration even if that's not likely a security issue.
Comment 8 commit-hook freebsd_committer freebsd_triage 2024-08-08 20:07:39 UTC
A commit in branch stable/14 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=33708452aaabca205d81eceb83e0813e5882815c

commit 33708452aaabca205d81eceb83e0813e5882815c
Author:     Kyle Evans <kevans@FreeBSD.org>
AuthorDate: 2024-08-05 18:43:56 +0000
Commit:     Kyle Evans <kevans@FreeBSD.org>
CommitDate: 2024-08-08 19:59:21 +0000

    calendar: don't setlogin(2) in the -a user handlers

    As of e67975d331 ("Fix 'calendar -a' in several ways."), `calendar -a`
    will now fork off a new process for each user and do all of its own
    processing in the user's own context.

    As a side-effect, calendar(1) started calling setlogin(2) in each of the
    forked processes and inadvertently hijacked the login name for the
    session it was running under, which was typically not a fresh session
    but rather that of whatever cron/periodic run spawned it.  Thus, daily
    and security e-mails started coming from completely arbitrary user.

    We could create a new session, but it appears that nothing calendar(1)
    does really needs the login name to be clobbered; opt to just avoid the
    setlogin(2) call entirely rather than incur the overhead of a new
    session for each process.

    PR:             280418
    Reviewed by:    des, olce
    Fixes:          e67975d331 ("Fix 'calendar -a' in several ways.")

    (cherry picked from commit 6cb8b61efe8899ee9194563108d0ae90c1eb89e3)

 usr.bin/calendar/calendar.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Comment 9 commit-hook freebsd_committer freebsd_triage 2024-08-08 20:07:40 UTC
A commit in branch stable/13 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=3a9010c98b3d9676307fac20d42cdd3cfd4bc46d

commit 3a9010c98b3d9676307fac20d42cdd3cfd4bc46d
Author:     Kyle Evans <kevans@FreeBSD.org>
AuthorDate: 2024-08-05 18:43:56 +0000
Commit:     Kyle Evans <kevans@FreeBSD.org>
CommitDate: 2024-08-08 20:00:40 +0000

    calendar: don't setlogin(2) in the -a user handlers

    As of e67975d331 ("Fix 'calendar -a' in several ways."), `calendar -a`
    will now fork off a new process for each user and do all of its own
    processing in the user's own context.

    As a side-effect, calendar(1) started calling setlogin(2) in each of the
    forked processes and inadvertently hijacked the login name for the
    session it was running under, which was typically not a fresh session
    but rather that of whatever cron/periodic run spawned it.  Thus, daily
    and security e-mails started coming from completely arbitrary user.

    We could create a new session, but it appears that nothing calendar(1)
    does really needs the login name to be clobbered; opt to just avoid the
    setlogin(2) call entirely rather than incur the overhead of a new
    session for each process.

    PR:             280418
    Reviewed by:    des, olce
    Fixes:          e67975d331 ("Fix 'calendar -a' in several ways.")

    (cherry picked from commit 6cb8b61efe8899ee9194563108d0ae90c1eb89e3)

 usr.bin/calendar/calendar.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Comment 10 commit-hook freebsd_committer freebsd_triage 2024-08-14 03:37:58 UTC
A commit in branch releng/13.4 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=7088bf662d46ef321a5838d290285086cd6446d2

commit 7088bf662d46ef321a5838d290285086cd6446d2
Author:     Kyle Evans <kevans@FreeBSD.org>
AuthorDate: 2024-08-05 18:43:56 +0000
Commit:     Kyle Evans <kevans@FreeBSD.org>
CommitDate: 2024-08-14 03:36:52 +0000

    calendar: don't setlogin(2) in the -a user handlers

    As of e67975d331 ("Fix 'calendar -a' in several ways."), `calendar -a`
    will now fork off a new process for each user and do all of its own
    processing in the user's own context.

    As a side-effect, calendar(1) started calling setlogin(2) in each of the
    forked processes and inadvertently hijacked the login name for the
    session it was running under, which was typically not a fresh session
    but rather that of whatever cron/periodic run spawned it.  Thus, daily
    and security e-mails started coming from completely arbitrary user.

    We could create a new session, but it appears that nothing calendar(1)
    does really needs the login name to be clobbered; opt to just avoid the
    setlogin(2) call entirely rather than incur the overhead of a new
    session for each process.

    PR:             280418
    Reviewed by:    des, olce
    Approved by:    re (cperciva)
    Fixes:          e67975d331 ("Fix 'calendar -a' in several ways.")

    (cherry picked from commit 6cb8b61efe8899ee9194563108d0ae90c1eb89e3)
    (cherry picked from commit 3a9010c98b3d9676307fac20d42cdd3cfd4bc46d)

 usr.bin/calendar/calendar.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Comment 11 Philip Paeps freebsd_committer freebsd_triage 2024-08-14 05:50:46 UTC
We discussed this one during the secteam fortnightly call earlier this week.  We don't mind issuing an erratum for this one, if someone could send us the filled in template:
https://www.freebsd.org/security/errata-template.txt
Comment 12 Kyle Evans freebsd_committer freebsd_triage 2024-08-16 22:07:18 UTC
Created attachment 252833 [details]
Proposed errata notice
Comment 13 Philip Paeps freebsd_committer freebsd_triage 2024-08-18 04:43:45 UTC
Thanks.  I've put your draft in the secteam repository together with the patch.  It appears to apply cleanly to all supported branches.  We'll include this one with our next batch of announcements.  We don't have a date planned yet.
Comment 14 commit-hook freebsd_committer freebsd_triage 2024-09-04 20:30:30 UTC
A commit in branch releng/13.3 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=eab94c0fbb782f1d849981d0dde2106ec9bc8df3

commit eab94c0fbb782f1d849981d0dde2106ec9bc8df3
Author:     Kyle Evans <kevans@FreeBSD.org>
AuthorDate: 2024-08-05 18:43:56 +0000
Commit:     Ed Maste <emaste@FreeBSD.org>
CommitDate: 2024-09-04 20:28:41 +0000

    calendar: don't setlogin(2) in the -a user handlers

    As of e67975d331 ("Fix 'calendar -a' in several ways."), `calendar -a`
    will now fork off a new process for each user and do all of its own
    processing in the user's own context.

    As a side-effect, calendar(1) started calling setlogin(2) in each of the
    forked processes and inadvertently hijacked the login name for the
    session it was running under, which was typically not a fresh session
    but rather that of whatever cron/periodic run spawned it.  Thus, daily
    and security e-mails started coming from completely arbitrary user.

    We could create a new session, but it appears that nothing calendar(1)
    does really needs the login name to be clobbered; opt to just avoid the
    setlogin(2) call entirely rather than incur the overhead of a new
    session for each process.

    PR:             280418
    Reviewed by:    des, olce
    Fixes:          e67975d331 ("Fix 'calendar -a' in several ways.")

    (cherry picked from commit 6cb8b61efe8899ee9194563108d0ae90c1eb89e3)
    (cherry picked from commit 3a9010c98b3d9676307fac20d42cdd3cfd4bc46d)

    Approved by:    so

 usr.bin/calendar/calendar.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Comment 15 commit-hook freebsd_committer freebsd_triage 2024-09-04 20:54:52 UTC
A commit in branch releng/14.0 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=d94dbaa516e0c7fb05a52f1c76020165482b7bf0

commit d94dbaa516e0c7fb05a52f1c76020165482b7bf0
Author:     Kyle Evans <kevans@FreeBSD.org>
AuthorDate: 2024-08-05 18:43:56 +0000
Commit:     Ed Maste <emaste@FreeBSD.org>
CommitDate: 2024-09-04 20:31:04 +0000

    calendar: don't setlogin(2) in the -a user handlers

    As of e67975d331 ("Fix 'calendar -a' in several ways."), `calendar -a`
    will now fork off a new process for each user and do all of its own
    processing in the user's own context.

    As a side-effect, calendar(1) started calling setlogin(2) in each of the
    forked processes and inadvertently hijacked the login name for the
    session it was running under, which was typically not a fresh session
    but rather that of whatever cron/periodic run spawned it.  Thus, daily
    and security e-mails started coming from completely arbitrary user.

    We could create a new session, but it appears that nothing calendar(1)
    does really needs the login name to be clobbered; opt to just avoid the
    setlogin(2) call entirely rather than incur the overhead of a new
    session for each process.

    PR:             280418
    Reviewed by:    des, olce
    Fixes:          e67975d331 ("Fix 'calendar -a' in several ways.")

    (cherry picked from commit 6cb8b61efe8899ee9194563108d0ae90c1eb89e3)
    (cherry picked from commit 33708452aaabca205d81eceb83e0813e5882815c)

    Approved by:    so

 usr.bin/calendar/calendar.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Comment 16 commit-hook freebsd_committer freebsd_triage 2024-09-04 21:35:22 UTC
A commit in branch releng/14.1 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=86d01789bf412b216b11021c44b60f93f6af8955

commit 86d01789bf412b216b11021c44b60f93f6af8955
Author:     Kyle Evans <kevans@FreeBSD.org>
AuthorDate: 2024-08-05 18:43:56 +0000
Commit:     Gordon Tetlow <gordon@FreeBSD.org>
CommitDate: 2024-09-04 21:31:57 +0000

    calendar: don't setlogin(2) in the -a user handlers

    As of e67975d331 ("Fix 'calendar -a' in several ways."), `calendar -a`
    will now fork off a new process for each user and do all of its own
    processing in the user's own context.

    As a side-effect, calendar(1) started calling setlogin(2) in each of the
    forked processes and inadvertently hijacked the login name for the
    session it was running under, which was typically not a fresh session
    but rather that of whatever cron/periodic run spawned it.  Thus, daily
    and security e-mails started coming from completely arbitrary user.

    We could create a new session, but it appears that nothing calendar(1)
    does really needs the login name to be clobbered; opt to just avoid the
    setlogin(2) call entirely rather than incur the overhead of a new
    session for each process.

    PR:             280418
    Reviewed by:    des, olce
    Fixes:          e67975d331 ("Fix 'calendar -a' in several ways.")

    (cherry picked from commit 6cb8b61efe8899ee9194563108d0ae90c1eb89e3)
    (cherry picked from commit 33708452aaabca205d81eceb83e0813e5882815c)

    Approved by:    so

 usr.bin/calendar/calendar.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Comment 17 Philip Paeps freebsd_committer freebsd_triage 2024-09-09 01:41:56 UTC
Erratum issued:
https://www.freebsd.org/security/advisories/FreeBSD-EN-24:15.calendar.asc