Dear Maintainer, * What led up to the situation? 1. A user ran "crontab -e" 2. He added the line (note the space): MAILTO=a@example.org, b@example.com 3. He saved and exited 4. No errors were reported to the user. * What was the outcome of this action? Subsequently, jobs ran but output was discarded. /var/log/syslog contains "UNSAFE MAIL" messages which the user cannot see. * What outcome did you expect instead? At step 4, crontab should have reported an error to the user and not applied the update.
Hello, Thank you for your bug report. I shall lower the severity of your bug report, since it "causes non-serious data loss" a@example.org would be parsed as a valid e-mail address if one uses regexp matching. What improvement would you suggest? Should "crontab -e" send an e-mail to recipients stated by MAILTO and wait for a reply? Best regards, Georges.
Sorry, my mail server does not seem to have received any email from debian when you sent your email on 2024-01-21. Was I supposed to have been automatically Bcc'd? I disagree that the bug is not grave - I believe it meets the criterion of data being lost (and was in fact lost by the user). However, that does not really bother me. Note that I used quotation marks around the word unsafe because that is the wording used in the syslog message; the addresses are not unsafe. The problem is the space character. If you try to replicate my test, you will see that after adding the MAILTO line shown in my report, no error is displayed to the user. Later when the job runs, syslog receives an error and job output is discarded. This happens because of the erroneous space delimiter, not because of any unusual email address. I am suggesting that instead of waiting until the job runs (when it may be too late to notify the user), the check that is reported in syslog be performed when saving after editing, so that the error can be reported to the user immediately. To be clear, I'm not asking for cron to be "more clever than its users", but that it runs earlier the tests that it already performs. Refusing to save a crontab that would fail later avoids potential data loss.
Perhaps amusingly, now I look at the code, I see in do_command.c that if safe_p() determines mailfrom contains spaces or parentheses, it will error out. However the very next line of code then sets it to contain both spaces and parentheses! (Yes, I know it is safe.) My suggestion is to call the sanity checks that already happen at runtime also at save time. If any fail, report them and deny save. My C is extremely rusty but if I understand the code, it seems checking happens in crontab.c in replace_cmd(). So one possibility might be: after load_env(), in the "case TRUE" clause, if envstr is MAILTO=x or MAILFROM=x, then call a safe_p() equivalent on x that invokes check_error() on failure instead of calling log_it(). To avoid code duplication, perhaps it would be better to generalise safe_p() with an extra argument to select how it reports errors (or do dependency injection of the logging function). I haven't looked to see what (if any) other sanity checks happen at runtime that could also usefully be tested at save time.
BTW, although testing validity of email would indeed be nice, it is notoriously difficult. For example, this is valid: http://@jhnc.org; ( https://www.rfc-editor.org/rfc/rfc5322#section-3.4 ) but I believe fails the regex in your link (I can't test properly without turning off my adblocker, which I don't intend to do). The safe_p() function could perhaps be generalised to recognise a list of valid addresses separated by `\s*,\s*` but beyond that it is almost certainly, as you say: "asking a program to be more clever than its users is a waste of energy". My concern is that whatever checks are done, they get done at save, so any "errors" are caught early.
We believe that the bug you reported is fixed in the latest version of
cron, which is due to be installed in the Debian FTP archive.
A summary of the changes between this version and the previous one is
attached.
Thank you for reporting the bug, which will now be closed. If you
have further comments please address them to 1061155@bugs.debian.org,
and the maintainer will reopen the bug report if appropriate.
Debian distribution maintenance software
pp.
Georges Khaznadar <georgesk@debian.org> (supplier of updated cron package)
(This message was generated automatically at their request; if you
believe that there is a problem with it please contact the archive
administrators by mailing ftpmaster@ftp-master.debian.org)
Format: 1.8
Date: Wed, 28 Feb 2024 16:27:09 +0100
Source: cron
Architecture: source
Version: 3.0pl1-186
Distribution: unstable
Urgency: medium
Maintainer: Georges Khaznadar <georgesk@debian.org>
Changed-By: Georges Khaznadar <georgesk@debian.org>
Closes: 1061155
Changes:
cron (3.0pl1-186) unstable; urgency=medium
.
* modified the attribution of patch Wrap-long-lines.patch, it was
authored by Frank Heckenbach <f.heckenbach@fh-soft.de>, while
Harald Dunkel reported the bug fixed by Frank Heckenbach's patch
* reorganized the source about function safe_p, so it can be used
in the file crontab.c, and let emit a warning when variables
MAITO or MAILFROM are defined "unsafely". Closes: #1061155
Checksums-Sha1:
743a3c3080045b143699e484c25a1fc122cf33ec 2060 cron_3.0pl1-186.dsc
a40265372c459c937b4d2a37c7f74bccf69e4ebb 127936 cron_3.0pl1-186.debian.tar.xz
db4b968ac6b96c6519352d1704d2577ae7f5f803 7465 cron_3.0pl1-186_amd64.buildinfo
Checksums-Sha256:
609d0426ba832d8098e39ad024e03f16379ef7ac3f2547cef247e8482b2be845 2060 cron_3.0pl1-186.dsc
556e0083093e64772b9aeed2b80acb366dab078b95c3e27920bb08644d92a95c 127936 cron_3.0pl1-186.debian.tar.xz
ac7f48ca53953abfa51b505f328b52229c95c23db2465ff3858253c5b3c719ed 7465 cron_3.0pl1-186_amd64.buildinfo
Files:
e84be5b197264df09792d0ac5cdee17c 2060 admin important cron_3.0pl1-186.dsc
8ebbabbcd372faf3d12e899c230c6a7c 127936 admin important cron_3.0pl1-186.debian.tar.xz
a44f3532c4f0b159956dd8421a98c0f1 7465 admin important cron_3.0pl1-186_amd64.buildinfo
-----BEGIN PGP SIGNATURE-----
iQIzBAEBCAAdFiEEM0CzZP9nFT+3zK6FHCgWkHE2rjkFAmXffu0ACgkQHCgWkHE2
rjmV4w/+NPkvjmdH0KveZqqozhl/5D7MZGhMG8wZIpS7TyzrDm3xznU3KmXAiqmm
UiG4vHpM5JEgPfrwAyGSlf/tftiqvRBaiouPyoYqT3GgtVS+AnK3OnDWfsoLHGX8
AFpeD6jLxwAcIaqVuEOKpnYMpIbT0B6tzMQKM/ZHEJPWo/oFaTaIKettIB/y7BtU
YwdonrWFPiI8+qaixCMtPgBEkTClkylaHpTxM8hgDKr8sMBuCeOl1iD5+zOhfgV1
y0wPmtr51/PquZGyd5hB6Erlsk84KnwmOkLwF58e+3HaI4SayOkkuoyYE6NMNWdL
q52BVNGIj3+GX/eONxHDx/llBhoPOLSzTFxkewUXJGAhSecYScRUy2QfiYfRqhT6
zXpYoYhZIbuQ5pbgykbVDnGlAqigc9WP6DOkiTwRCsi8O5TffNRnkyG7GxSrv18o
Hw77MSQGL5Fr2TiTlo7tM2Vw6nGJHgqboVr/8n1YAd0CIq5IeFy4RIAz3jHJSQA7
RUG7rG1Jr5WOmbmbPjmIDxpj4CzITm4DJcEdm7D3YxmimIiWYeMwT2kI8RV1euJw
eUUAjUZ+FFcjfl2MJUf9KlaUQDHiXJZnOxgbJzNMv0O3WaesGVibxDmSuL8UKrdq
0waJ/Qgy03cOCPWcaDqJ0RRj2+n2/Run++8Yg26NEZqV4nC/M2Y=
=aAlu
-----END PGP SIGNATURE-----
Hi, I just received the new package and tried it. Thanks.
It detects unacceptable MAILTO/MAILFROM, but because unacceptable
values will cause an error later, issuing only a warning feels
inadequate to me.
For usability, perhaps it would be better to use check_error().
Currently, warnings could be missed since the exit status with
`-n` is still 0.
Something like:
case TRUE:
/* here MAILTO and MAILFROM are checked */
if (
strncmp(envstr, "MAILTO=", 7) == 0 ||
strncmp(envstr, "MAILFROM=", 9) == 0
){
if (! safe_p("", strstr(envstr,"=")+1)){
check_error("unsafe mail");
}
}
break;
The current safe_p() implementation may cause a syslog entry to be
generated with no associated username when called here, which feels
slightly wrong to me. It could be confusing to someone auditing logs
to see spurious "() UNSAFE MAIL" messages when `-n` is used.
Dear Jonathan, what is the right message to feed back to the user? Warning ? +---------+ | Warning | ? +---------+ +-----------------------------------------+ | +-------------------------------------+ | | | POOR MISGUIDED, YOU MADE AN ERROR ! | | ? | +-------------------------------------+ | +-----------------------------------------+ All in all, if the user does not want to read feedback messages, I ignore how to tell them the proper way. About the exit status with -n: if one runs `crontab -n`, one expects somme feedback message, because nothing else is going to happen. The exit status is just an integer, far less expressive than a warning which points out some defect. Do you always check "$?" when you get an error or a warning message? Jonathan H N Chin a écrit :
Since there is an existing "standard" for crontab error reporting ( check_error() diagnostic + non-zero exit ), I am suggesting not inventing a new one ( warning message + zero exit ). The crontab(1) manpage does not mention exit code; but it is very common for zero to mean success / non-zero to mean failure. I have no idea how other people use crontab, but I can easily imagine someone has written automation code along the lines of: if crontab -n newfile 2>&-; then # looks good, do it for real now crontab - <newfile else : oops, do some error handling fi For example: https://github.com/sourcemage/grimoire/blob/9c3932d181e16dc2c0320f7bb189e42d445fd4d7/utils/fcrontabs/fcrontab#L25 https://github.com/brsf11/mugen-riscv/blob/9c474e23beb4834d6d87b7434edf94e0689d6a8d/testcases/cli-test/crontabs/oe_test_crontabs.sh#L56 I would like to support users; not belittle them.
Jonathan H N Chin a écrit : Looks fine; I shall use check_error() in the future release (3.0pl1-187) Best regards, Georges.