Page MenuHomeFreeBSD

newsyslog: Wrap months when parsing 'W' rules.
AbandonedPublic

Authored by jhb on Dec 27 2021, 2:54 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Apr 27, 5:24 AM
Unknown Object (File)
Mon, Apr 22, 1:52 AM
Unknown Object (File)
Mar 12 2024, 2:13 AM
Unknown Object (File)
Dec 20 2023, 2:43 AM
Unknown Object (File)
Oct 30 2023, 5:56 AM
Unknown Object (File)
Sep 17 2023, 7:50 AM
Unknown Object (File)
Mar 5 2023, 8:01 AM
Unknown Object (File)
Feb 16 2023, 12:11 PM
Subscribers

Details

Reviewers
gad
jrtc27
Summary

When parsing a rule to rotate log files on a specific week day,
parseDWM() can advance the time to the next week. If the next week is
in the next month, then tm_mon is incremented. However, the increment
was failing to handle the wraparound from December to January, so when
parsing a rule during the last week of the December, the month would
advance to month 12. This triggered an out-of-bounds read of the
mtab[] array in days_pmonth() after parseDWM() returned.

The default rule for /var/log/weekly.conf triggers this during the
last week of December each year.

Reported by: CHERI
Obtained from: CheriBSD
Sponsored by: The University of Cambridge, Google Inc.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 43606
Build 40494: arc lint + arc unit

Event Timeline

jhb requested review of this revision.Dec 27 2021, 2:54 AM

In an amazing coincidence, on Nov 30th I fixed a bug in a program of my own (totally unrelated to this) which had a similar bug. But in that case the bug happened only when the program was run during the last hour of a month. That program was eight years old, and it wasn't until Nov 30th 2021 that the bug stung me.

I'd want to do some testing, but I suspect this is not complete. Wouldn't this have to check for the value of 12, and if that is hit then *also* increment the year? (in addition to resetting the month to zero) And yes, the first fix for my own program caused the time to be reset to Nov 1st, instead of Dec 1st...

And after checking, it is true that the value for tm_year must also be fixed. Also, it's the default entry for /var/log/weekly.log where this issue comes up, not /var/log/weekly.conf . Now let's see if I can remember how to upload a new patch file to review {F34421954}for this. This is a patch based on branch 13-stable, which I assume will work fine for freebsd-current.

Hmm. Looks like that wasn't quite right. Let me try another tactic.

Isn't there a routine to normalize the tm so that all the edge cases are neatly taken care of?

In D33666#761239, @imp wrote:

Isn't there a routine to normalize the tm so that all the edge cases are neatly taken care of?

Not that I'm aware of, but then I have not written much code which does arithmetic with tm-values in C. It's possible that this code in newsyslog is the only time I've ever done that!

Wrt 'reviews" per se, do I have to create a new "review diff" instead of just updating the diff this one?

Well, I created a new review-entry, D33687

usr.sbin/newsyslog/ptimes.c
280

I realized this isn't quite right and in the case that tm_mon overflows we need to bump the year as well.

Sorry, for some reason my review comment about needing to bump the year didn't go through (I originally added it a few ours after I uploaded this).