Page MenuHomeFreeBSD

lpd: Improve robustness
ClosedPublic

Authored by des on Fri, Feb 20, 4:03 PM.
Tags
None
Referenced Files
F146066023: D55399.id.diff
Fri, Feb 27, 12:30 PM
F145978590: D55399.id172749.diff
Thu, Feb 26, 5:30 PM
F145962669: D55399.id172749.diff
Thu, Feb 26, 2:11 PM
Unknown Object (File)
Wed, Feb 25, 10:00 PM
Unknown Object (File)
Wed, Feb 25, 1:58 PM
Unknown Object (File)
Wed, Feb 25, 12:05 PM
Unknown Object (File)
Wed, Feb 25, 11:46 AM
Unknown Object (File)
Wed, Feb 25, 11:40 AM

Details

Summary
  • Check for integer overflow when receiving file sizes.
  • Check for buffer overflow when receiving file names, and fully validate the names.
  • Check for integer overflow when checking for available disk space.
  • Check for I/O errors when sending status codes.
  • Enforce one job per connection and one control file per job (see code comments for additional details).
  • Simplify readfile(), avoiding constructs vulnerable to integer overflow.
  • Don't delete files we didn't create.
  • Rename read_number() to read_minfree() since that's all it's used for, and move all the minfree logic into it.
  • Fix a few style issues.

PR: 293278
MFC after: 3 days

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 70967
Build 67850: arc lint + arc unit

Event Timeline

des requested review of this revision.Fri, Feb 20, 4:03 PM
markj added inline comments.
usr.sbin/lpr/lpd/recvjob.c
209

This validation is new AFAICS. It probably deserves to be mentioned explicitly in the commit log message.

260

Same comment here with respect to the commit log message.

293–297
332

We're reading stdout...?

This revision is now accepted and ready to land.Sat, Feb 21, 2:52 AM
usr.sbin/lpr/lpd/recvjob.c
209

It's new but in line with RFC 1179 and necessary. I didn't realize this until just now (and neither did the reporter) but without this check, lpd will let any client upload files to the print spool that never get picked up for processing and never get deleted.

293–297

Normally I'd agree but this is the established style here (and in a lot of CSRG code).

332

Yes, that's our TCP socket, see inside the fork() == 0 block in lpd.c.

don't delete files we didn't create

This revision now requires review to proceed.Sat, Feb 21, 2:11 PM
markj added inline comments.
usr.sbin/lpr/lpd/recvjob.c
399

This will fail if size is a multiple of 512 and avail - minfree == size / 512, which seems wrong.

This revision is now accepted and ready to land.Sat, Feb 21, 3:37 PM
usr.sbin/lpr/lpd/recvjob.c
209

...and fwiw it's already mentioned in the commit log: “fully validate the names”.

399

It's an intentional omission. To handle this situation correctly I would either have to add a special case just for when size is a multiple of 512 or deal with the possible overflow in (size + 511) / 512. The whole thing is best-effort anyway and easily circumvented, so I chose not to deal with it.

This revision now requires review to proceed.Sat, Feb 21, 4:34 PM

really fix multi-file jobs

usr.sbin/lpr/lpd/recvjob.c
158–159

Did you mean to leave this in?

265

Should this be kept?

des marked 7 inline comments as done.Tue, Feb 24, 5:39 PM
des added inline comments.
usr.sbin/lpr/lpd/recvjob.c
158–159

grrr no, thank you for catching it

des marked an inline comment as done.Tue, Feb 24, 5:39 PM

I'm not sure about a 3 day MFC timeout, but I'm also not sure that any significant number of people are using this code on main.

This revision is now accepted and ready to land.Wed, Feb 25, 9:02 PM

I'm not sure about a 3 day MFC timeout, but I'm also not sure that any significant number of people are using this code on main.

I assumed that we'd want to merge it quickly to 15 and especially 14...

In D55399#1270409, @des wrote:

I'm not sure about a 3 day MFC timeout, but I'm also not sure that any significant number of people are using this code on main.

I assumed that we'd want to merge it quickly to 15 and especially 14...

Given how long the bugs have been there, I wouldn't rush, but it is up to you. I expect Colin would be apprehensive about taking this patch into 14.4 at this point.

This revision was automatically updated to reflect the committed changes.