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.

Sign up for GitHub

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

Jump to bottom

Enabling audit causes tests to fail #1028

Closed
siteshwar opened this issue Nov 28, 2018 · 2 comments
Closed

Enabling audit causes tests to fail #1028

siteshwar opened this issue Nov 28, 2018 · 2 comments
Assignees
@siteshwar
Labels
bug

Comments

@siteshwar
Copy link
Contributor

Description of problem:
Enabling audit causes tests to fail

Ksh version:
2017.0.0-devel-2060-g68fad3ba

How reproducible:
Always

Steps to reproduce:

  1. Set content of /etc/ksh_audit to /tmp/ksh_auditfile;<your user id>
  2. meson build; ninja -C build
  3. cd build; meson test

Actual results:
Some tests fail due to crash.

Expected results:
All tests should pass.
@siteshwar siteshwar added the bug label Nov 28, 2018
@siteshwar
Copy link
Contributor Author

Once this is fixed, it would make sense to test auditing in CI by creating /etc/ksh_audit file in every build.

@siteshwar siteshwar self-assigned this Dec 9, 2018
@siteshwar siteshwar mentioned this issue Dec 9, 2018
Merged
siteshwar added a commit that referenced this issue Dec 9, 2018
@siteshwar
Fix a crash when auditing is enabled
ac6770c 
`ttyname()` returns NULL if a fd is not associated with any terminal.
This may cause a crash in `strdup()` function.

Resolves: #1028
@krader1961
Copy link
Contributor

FWIW, Coverity Scan CID 287610 points out a serious security bug in the command history auditing code. As I've argued elsewhere this feature is useful in theory but the ksh implementation is so insecure as to be no more trustworthy than email sent by telneting to a SMTP server two decades ago.

JohnoKing added a commit to JohnoKing/ksh that referenced this issue Jul 6, 2020
@JohnoKing
Fix use of strdup on a NULL pointer
aaf0442 
The following set of commands can rarely cause a memory fault,
although most of the time it will simply cause ksh to write
'(null)' to the ksh auditing file:

$ [ -e /etc/ksh_audit ] || echo "/tmp/ksh_auditfile;$(id -u)" | sudo tee /etc/ksh_audit;
$ v=$(ksh  2> /dev/null +o rc -ic $'getopts a:bc: opt --man\nprint $?')
$ cat /tmp/ksh_auditfile
1000;1593599493;(null); getopts a:bc: opt --man

This happens because 'ttyname' can return NULL, which results in
undefined behavior when used with 'strdup'.
See att#1028

src/cmd/ksh93/edit/history.c:
- Make strdup duplicate 'notty' instead of NULL to prevent
  crashes.

.github/workflows/ci.yml:
- Create /etc/ksh_audit in the CI builds to test for crashes
  when auditing is enabled.
JohnoKing added a commit to JohnoKing/ksh that referenced this issue Jul 6, 2020
@JohnoKing
Fix use of strdup on a NULL pointer
d98b1b5 
The following set of commands can rarely cause a memory fault,
although most of the time it will simply cause ksh to write
'(null)' to the ksh auditing file:

$ [ -e /etc/ksh_audit ] || echo "/tmp/ksh_auditfile;$(id -u)" | sudo tee /etc/ksh_audit;
$ v=$(ksh  2> /dev/null +o rc -ic $'getopts a:bc: opt --man\nprint $?')
$ cat /tmp/ksh_auditfile
1000;1593599493;(null); getopts a:bc: opt --man

This happens because 'ttyname' can return NULL, which results in
undefined behavior when used with 'strdup'.
See att#1028

src/cmd/ksh93/edit/history.c:
- Make strdup duplicate 'notty' instead of NULL to prevent
  crashes.
JohnoKing added a commit to JohnoKing/ksh that referenced this issue Jul 6, 2020
@JohnoKing
Fix use of strdup on a NULL pointer
3d2b66c 
The following set of commands can rarely cause a memory fault
when auditing is enabled, although most of the time it will
simply cause ksh to write '(null)' to the auditing file in place
of a tty name:

$ [ -e /etc/ksh_audit ] || echo "/tmp/ksh_auditfile;$(id -u)" | sudo tee /etc/ksh_audit;
$ v=$(ksh  2> /dev/null +o rc -ic $'getopts a:bc: opt --man\nprint $?')
$ cat /tmp/ksh_auditfile
1000;1593599493;(null); getopts a:bc: opt --man

This happens because strdup is used unconditionally on the pointer
returned by 'ttyname', which can be NULL if stderr is closed. The
ksh implementation of strdup then attempts to get the length of
NULL with strlen, which leads to undefined behavior.
See att#1028

src/cmd/ksh93/edit/history.c:
- Make strdup duplicate 'notty' instead of NULL to prevent
  crashes.
JohnoKing added a commit to JohnoKing/ksh that referenced this issue Jul 6, 2020
@JohnoKing
Fix use of strdup on a NULL pointer
4310896 
The following set of commands can rarely cause a memory fault
when auditing is enabled, although most of the time it will
simply cause ksh to write '(null)' to the auditing file in place
of a tty name:

$ [ -e /etc/ksh_audit ] || echo "/tmp/ksh_auditfile;$(id -u)" | sudo tee /etc/ksh_audit;
$ v=$(ksh  2> /dev/null +o rc -ic $'getopts a:bc: opt --man\nprint $?')
$ cat /tmp/ksh_auditfile
1000;1593599493;(null); getopts a:bc: opt --man

This happens because strdup is used unconditionally on the pointer
returned by 'ttyname', which can be NULL if stderr is closed. The
ksh implementation of strdup then attempts to get the length of
NULL with strlen, which leads to undefined behavior.
See att#1028

src/cmd/ksh93/edit/history.c:
- Make strdup duplicate 'notty' instead of NULL to prevent
  crashes.
@JohnoKing JohnoKing mentioned this issue Jul 6, 2020
Merged
JohnoKing added a commit to JohnoKing/ksh that referenced this issue Jul 6, 2020
@JohnoKing
Fix use of strdup on a NULL pointer
fc10f9a 
The following set of commands can rarely cause a memory fault
when auditing is enabled, although most of the time it will
simply cause ksh to write '(null)' to the auditing file in place
of a tty name:

$ [ -e /etc/ksh_audit ] || echo "/tmp/ksh_auditfile;$(id -u)" | sudo tee /etc/ksh_audit;
$ v=$(ksh  2> /dev/null +o rc -ic $'getopts a:bc: opt --man\nprint $?')
$ cat /tmp/ksh_auditfile
1000;1593599493;(null); getopts a:bc: opt --man

This happens because strdup is used unconditionally on the pointer
returned by 'ttyname', which can be NULL if stderr is closed. This
then causes 'hp->tty' to be set to null, as strdup returns NULL.
See att#1028

src/cmd/ksh93/edit/history.c:
- Make strdup duplicate 'notty' instead of NULL to prevent
  crashes.
McDutchie pushed a commit to ksh93/ksh that referenced this issue Jul 6, 2020
@JohnoKing
Fix use of strdup on a NULL pointer (#63)
9a9da2c 
The following set of commands can rarely cause a memory fault
when auditing[*] is enabled, although most of the time it will
simply cause ksh to write '(null)' to the auditing file in place
of a tty name:

$ [ -e /etc/ksh_audit ] || echo "/tmp/ksh_auditfile;$(id -u)" | sudo tee /etc/ksh_audit;
$ v=$(ksh  2> /dev/null +o rc -ic $'getopts a:bc: opt --man\nprint $?')
$ cat /tmp/ksh_auditfile
1000;1593599493;(null); getopts a:bc: opt --man

This happens because strdup is used unconditionally on the pointer
returned by 'ttyname', which can be NULL if stderr is closed. This
then causes 'hp->tty' to be set to null, as strdup returns NULL.
See att#1028

src/cmd/ksh93/edit/history.c:
- Make strdup duplicate 'notty' instead of NULL to prevent
  crashes.

[*] https://blog.fpmurphy.com/2008/12/ksh93-auditing-and-accounting.html
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@siteshwar siteshwar

Labels
bug
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@siteshwar @krader1961