Re: utmps: ut_id field gets truncated

2021-04-12 Thread Xavier Stonestreet
On Tue, Apr 13, 2021 at 3:11 AM Laurent Bercot  wrote:
>
>   Please test the latest git head and tell me if it's working for you.

Yep, it's all good. Thanks for making the changes.

>   Anyway, I changed utmps so that ut_id - and also ut_user and ut_line,
> for consistency - are treated as char arrays, not as null-terminated
> strings.

I think that is the correct implementation. The spec doesn't make it
clear but it's implied that these fields are not C strings, they are
just arrays of characters. In the few packages that read from or write
to u/wtmp that I've looked at, they all have a bunch of boilerplate
code to fill in the record or extract info from it, and they all (I
may be overgeneralizing a bit here) treat these fields as non-null
terminated, relying either on sizeof() or the non-standard
UT_xSIZE macros as the max length of the field.

>   (This is not defensive programming, this is sanitization: the data is
> provided by some user and ends up in a global database, which may then
> be read by some other user. Sanitizing it is good practice.)
>
>   In other words, I reverted to GIGO. If a client puts in a non-null-
> terminated array and another client expects a null-terminating string
> and makes the mistake of trusting data coming from the utmp database,
> then hilarity will ensue. Too bad, they can't have it both ways.

Oh I totally understand where you're coming from and you initially did
what any sane C system library writer would do.


This utmpx interface is an abomination!

1) It doesn't use null-terminated strings. For a C API, that's
unintuitive, bothersome and plain stupid.
2) It exposes the raw database records, structure and data, directly
to the client.
3) No sanitization or validation whatsoever. No collision detection.
Any malicious or buggy client can shove anything in the database.

The way I see it, this was probably slapped together by some guy at
Bell Labs or Berkeley who was bored on a Sunday afternoon. That this
managed to find its way AS-IS into a fundamental standard is
mind-boggling. And what's up with the name? utmp, wtmp - what the heck
is that supposed to mean? Temporary, yeah, that might be. And yet here
we are 35 years later. Sigh.

I was pretty close to throwing this whole thing in the trash bin but
now that it's working I might as well keep it. Again I'm talking about
utmpx the interface and related tools, not about your utmps
implementation. And if nothing else I'm glad to have helped you a
little.

As a system administrator, I don't particularly want these tools and
their talk/write cousins being available to regular users - they are
indeed useless and privacy-invading nowadays. But I want to be able to
keep a real-time and historical record of which users are logged into
the machine, when, and from where. What alternatives are there without
u/wtmp? Scanning and parsing the syslog? LOL. No, thanks.



Re: utmps: ut_id field gets truncated

2021-04-12 Thread Laurent Bercot

As the title says. If you put "1234" in the ut_field of the utmpx,
call pututxline() and then read it back, you get "123" back. An od(1)
dump of the utmp file shows that what is stored is 1, 2, 3 and \0.


 Yup. Again, it comes down to a lack of specification.

 https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/utmpx.h.html
says that ut_user, ut_id and ut_line are all char[].
 Linux clients seem to interpret them as strings, so I treated them
as strings, in the safest way I could: making sure to add terminating
null characters at the end, just in case.

 (This is not defensive programming, this is sanitization: the data is
provided by some user and ends up in a global database, which may then
be read by some other user. Sanitizing it is good practice.)

 But from what you said, OpenSSH does not treat ut_id as a string.
It treats it as an array of 4 characters, that does not need to be
null-terminated. This is permitted by the specification, which
describes ut_id as "Unspecified initialization process identifier"
(thanks, spec, very helpful), but goes against the principle of
least surprise since it treats ut_user and ut_line as strings. Or
maybe it does not and it's just that user and line names are always
short enough to never cause any problem.

 Anyway, I changed utmps so that ut_id - and also ut_user and ut_line,
for consistency - are treated as char arrays, not as null-terminated
strings.
 In other words, I reverted to GIGO. If a client puts in a non-null-
terminated array and another client expects a null-terminating string
and makes the mistake of trusting data coming from the utmp database,
then hilarity will ensue. Too bad, they can't have it both ways.

 Please test the latest git head and tell me if it's working for you.

--
 Laurent



Re: utmps: ut_id field gets truncated

2021-04-12 Thread Amin Rasooli
Hi,

How can I opt-out if the subscription to this email chain? I don’t think it is 
related to me. 

Regards,
Amin Rasooli
Associate Director, AI Enablement and AI Ops 
Phone: 312.229.2540
 

On 4/11/21, 11:51 PM, "skaware@list.skarnet.org on behalf of Xavier 
Stonestreet"  
wrote:

THIS EMAIL WAS SENT BY SOMEONE OUTSIDE OF CCC. USE CAUTION BEFORE CLICKING 
ON ANY LINKS.


One more report, sorry...

As the title says. If you put "1234" in the ut_field of the utmpx,
call pututxline() and then read it back, you get "123" back. An od(1)
dump of the utmp file shows that what is stored is 1, 2, 3 and \0.

This is a problem for example with ssh (OpenSSH to be precise, I don't
know what Dropbear does) and pseudo-terminals. OpenSSH uses the last 4
characters of the pty as the id field. So for example if one user is
connected to /dev/pts/0, the id used for utmp is "ts/0". If another
user is connected to /dev/pts/1, the id is "ts/1". And so forth.

With the truncation glitch both entries are recorded with id "ts/".
Which should become an issue when it comes to searching for the right
record when one of the users logs out and utmp is updated, since they
both collide. And yet for some unfathomable reason the correct record
gets found and updated as per my limited testing.

I looked briefly at the code, client and server-side, but didn't see
anything obvious. There must be a simple off-by-1 calculation or
indexing error somewhere.

Thanks.