Re: [ros-dev] [ros-diffs] 01/01: [WINLOGON] Clean up part 2 - Replace the UNICODE_STRING usMessage by a PWSTR pszMessage. - Use the "%02d:%02d:%02d" time format and get rid of the safe string printf because the string will NEVER be longer than 8 characters. - Rename the timer id constant because it is NOT a resource id. - Rename variables according to the coding style.

[Thomas Faber]

Hey Eric,

On 2018-04-02 12:58, Eric Kohl wrote:
> RtlStringCbPrintfW(strbuf, sizeof(strbuf), L"%d:%d:%d", hours, minutes, seconds);
> swprintf(szBuffer, L"%02d:%02d:%02d", iHours, iMinutes, iSeconds);

Unfortunately I must disagree with this change.

Buffer overflows are a big enough threat that code review and
static analysis are not generally considered sufficient to protect
against them.
So it's best practice for new code to always verify sizes at run-time,
and never to use s(w)print.

Best regards,
Thomas

PS: from what I see, iHours can be as large as 1193046, which won't
     fit in 2 digits

[Eric Kohl]

Hello Thomas,

you're right, using the run-time size checks are a good way to keep
application from crashing because of buffer overflows. They'll just keep
on using corrupt data instead! If you want to fix this problem: Don't
use C! Use C++, C#, Java etc. instead!

I prefer to see an application crash because of a buffer overflow rather
than seeing it store truncated phone numbers in a database.

PS: If the timeout is longer than a day, winlogon uses the "%d days"
format. In the end, a buffer of 10 characters is still large enough.

PPS: I'll keep using the old functions until you remove them from the
runtime code.

Regards
Eric

Am 02.04.2018 um 14:12 schrieb Thomas Faber:
> Hey Eric,
>
> On 2018-04-02 12:58, Eric Kohl wrote:
> > -    RtlStringCbPrintfW(strbuf, sizeof(strbuf), L"%d:%d:%d", hours,
> > minutes, seconds);
> > +    swprintf(szBuffer, L"%02d:%02d:%02d", iHours, iMinutes, iSeconds);
>
> Unfortunately I must disagree with this change.
>
> Buffer overflows are a big enough threat that code review and
> static analysis are not generally considered sufficient to protect
> against them.
> So it's best practice for new code to always verify sizes at run-time,
> and never to use s(w)print.
>
> Best regards,
> Thomas
>
> PS: from what I see, iHours can be as large as 1193046, which won't
>     fit in 2 digits
>
> Ros-dev mailing list
> [e-mail]
> http://www.reactos.org/mailman/listinfo/ros-dev

[Magnus Johnsson]

Eric, the thing is, buffer overflows don't just crash the program unless
you have some really nifty guard pages, but overwrite other things in
memory. This means an attacker can, in certain situations, use it to create
something that not just crashes, but with a nifty input create an exploit.
Having a 'Oh it will just crash' attitude is 'not the best'.

2018-04-02 19:41 GMT+02:00 Eric Kohl [e-mail]:
> Hello Thomas,
>
> you're right, using the run-time size checks are a good way to keep
> application from crashing because of buffer overflows. They'll just keep
> on using corrupt data instead! If you want to fix this problem: Don't
> use C! Use C++, C#, Java etc. instead!
>
> I prefer to see an application crash because of a buffer overflow rather
> than seeing it store truncated phone numbers in a database.
>
> PS: If the timeout is longer than a day, winlogon uses the "%d days"
> format. In the end, a buffer of 10 characters is still large enough.
>
> PPS: I'll keep using the old functions until you remove them from the
> runtime code.
>
> Regards
> Eric
>
> Am 02.04.2018 um 14:12 schrieb Thomas Faber:
> > Hey Eric,
> >
> > On 2018-04-02 12:58, Eric Kohl wrote:
> > > RtlStringCbPrintfW(strbuf, sizeof(strbuf), L"%d:%d:%d", hours,
> > >
> > > minutes, seconds);
> > > swprintf(szBuffer, L"%02d:%02d:%02d", iHours, iMinutes, iSeconds);
> >
> > Unfortunately I must disagree with this change.
> >
> > Buffer overflows are a big enough threat that code review and
> > static analysis are not generally considered sufficient to protect
> > against them.
> > So it's best practice for new code to always verify sizes at run-time,
> > and never to use s(w)print.
> >
> > Best regards,
> > Thomas
> >
> > PS: from what I see, iHours can be as large as 1193046, which won't
> >     fit in 2 digits
> >
> > Ros-dev mailing list
> > [e-mail]
> > http://www.reactos.org/mailman/listinfo/ros-dev
>
> Ros-dev mailing list
> [e-mail]
> http://www.reactos.org/mailman/listinfo/ros-dev

[Thomas Faber]

Finding bugs is definitely a valid concern. But there is, of course,
a version that addresses both problems:
NT_VERIFY(NT_SUCCESS(RtlStringCbPrintfW(...)));

This will assert in case the buffer is too small, while still never
causing an overflow.

We could provide wrappers to require less typing or raise an exception
even on release builds, if that's more desirable.

Best,
Thomas

On 2018-04-02 20:28, Magnus Johnsson wrote:
> Eric, the thing is, buffer overflows don't just crash the program unless
> you have some really nifty guard pages, but overwrite other things in
> memory. This means an attacker can, in certain situations, use it to create
> something that not just crashes, but with a nifty input create an exploit.
> Having a 'Oh it will just crash' attitude is 'not the best'.
>
> 2018-04-02 19:41 GMT+02:00 Eric Kohl [e-mail]:
> > Hello Thomas,
> >
> > you're right, using the run-time size checks are a good way to keep
> > application from crashing because of buffer overflows. They'll just keep
> > on using corrupt data instead! If you want to fix this problem: Don't
> > use C! Use C++, C#, Java etc. instead!
> >
> > I prefer to see an application crash because of a buffer overflow rather
> > than seeing it store truncated phone numbers in a database.
> >
> > PS: If the timeout is longer than a day, winlogon uses the "%d days"
> > format. In the end, a buffer of 10 characters is still large enough.
> >
> > PPS: I'll keep using the old functions until you remove them from the
> > runtime code.
> >
> > Regards
> > Eric
> >
> > Am 02.04.2018 um 14:12 schrieb Thomas Faber:
> > > Hey Eric,
> > >
> > > On 2018-04-02 12:58, Eric Kohl wrote:
> > > > RtlStringCbPrintfW(strbuf, sizeof(strbuf), L"%d:%d:%d", hours,
> > > >
> > > > minutes, seconds);
> > > > swprintf(szBuffer, L"%02d:%02d:%02d", iHours, iMinutes, iSeconds);
> > >
> > > Unfortunately I must disagree with this change.
> > >
> > > Buffer overflows are a big enough threat that code review and
> > > static analysis are not generally considered sufficient to protect
> > > against them.
> > > So it's best practice for new code to always verify sizes at run-time,
> > > and never to use s(w)print.
> > >
> > > Best regards,
> > > Thomas
> > >
> > > PS: from what I see, iHours can be as large as 1193046, which won't
> > >      fit in 2 digits

[Huw Campbell]

Hey,

I'm going to chime in here.

If you want to fix this problem: Don't use C! Use C++, C#, Java etc.
> instead!

Honestly, this is inane right? ReactOS currently uses C predominantly, but
we can't
use that as an excuse for shitty code which breaks, crashes, or has buffer
overflows.
Security issues are not ok.

Cheers,
Huw

On Tue, Apr 3, 2018 at 5:53 AM, Thomas Faber [e-mail]
wrote:
> Finding bugs is definitely a valid concern. But there is, of course,
> a version that addresses both problems:
> NT_VERIFY(NT_SUCCESS(RtlStringCbPrintfW(...)));
>
> This will assert in case the buffer is too small, while still never
> causing an overflow.
>
> We could provide wrappers to require less typing or raise an exception
> even on release builds, if that's more desirable.
>
> Best,
> Thomas
>
> On 2018-04-02 20:28, Magnus Johnsson wrote:
> > Eric, the thing is, buffer overflows don't just crash the program unless
> > you have some really nifty guard pages, but overwrite other things in
> > memory. This means an attacker can, in certain situations, use it to
>
> create
> > something that not just crashes, but with a nifty input create an
>
> exploit.
> > Having a 'Oh it will just crash' attitude is 'not the best'.
> >
> > 2018-04-02 19:41 GMT+02:00 Eric Kohl [e-mail]:
> > > Hello Thomas,
> > >
> > > you're right, using the run-time size checks are a good way to keep
> > > application from crashing because of buffer overflows. They'll just keep
> > > on using corrupt data instead! If you want to fix this problem: Don't
> > > use C! Use C++, C#, Java etc. instead!
> > >
> > > I prefer to see an application crash because of a buffer overflow rather
> > > than seeing it store truncated phone numbers in a database.
> > >
> > > PS: If the timeout is longer than a day, winlogon uses the "%d days"
> > > format. In the end, a buffer of 10 characters is still large enough.
> > >
> > > PPS: I'll keep using the old functions until you remove them from the
> > > runtime code.
> > >
> > > Regards
> > > Eric
> > >
> > > Am 02.04.2018 um 14:12 schrieb Thomas Faber:
> > > > Hey Eric,
> > > >
> > > > On 2018-04-02 12:58, Eric Kohl wrote:
> > > > > RtlStringCbPrintfW(strbuf, sizeof(strbuf), L"%d:%d:%d", hours,
> > > > >
> > > > > minutes, seconds);
> > > > > swprintf(szBuffer, L"%02d:%02d:%02d", iHours, iMinutes,
>
> iSeconds);
> > > > Unfortunately I must disagree with this change.
> > > >
> > > > Buffer overflows are a big enough threat that code review and
> > > > static analysis are not generally considered sufficient to protect
> > > > against them.
> > > > So it's best practice for new code to always verify sizes at run-time,
> > > > and never to use s(w)print.
> > > >
> > > > Best regards,
> > > > Thomas
> > > >
> > > > PS: from what I see, iHours can be as large as 1193046, which won't
> > > >      fit in 2 digits
>
> Ros-dev mailing list
> [e-mail]
> http://www.reactos.org/mailman/listinfo/ros-dev

[Riccardo Paolo Bestetti]

Some people use the very same argument to write OS’s in Rust. It’s not like it doesn’t make sense, it’s just not necessary.
If C++ and Rust can compile to safe code, so can C. You just gotta define and abide by sensible safety practices.

</stating_the_obvious>

BR,
Riccardo P. Bestetti

Da: Ros-dev [mailto:[e-mail]] Per conto di Huw Campbell
Inviato: giovedì 5 aprile 2018 13:35
A: ReactOS Development List [e-mail]
Oggetto: Re: [ros-dev] [ros-diffs] 01/01: [WINLOGON] Clean up part 2 - Replace the UNICODE_STRING usMessage by a PWSTR pszMessage. - Use the "%02d:%02d:%02d" time format and get rid of the safe string printf because the string will NEVER be longer than 8 charac...

Hey,

I'm going to chime in here.

If you want to fix this problem: Don't use C! Use C++, C#, Java etc. instead!

Honestly, this is inane right? ReactOS currently uses C predominantly, but we can't
use that as an excuse for shitty code which breaks, crashes, or has buffer overflows.
Security issues are not ok.

Cheers,
Huw

On Tue, Apr 3, 2018 at 5:53 AM, Thomas Faber <[e-mail]:[e-mail]> wrote:
Finding bugs is definitely a valid concern. But there is, of course,
a version that addresses both problems:
NT_VERIFY(NT_SUCCESS(RtlStringCbPrintfW(...)));

This will assert in case the buffer is too small, while still never
causing an overflow.

We could provide wrappers to require less typing or raise an exception
even on release builds, if that's more desirable.

Best,
Thomas

On 2018-04-02 20:28, Magnus Johnsson wrote:
> Eric, the thing is, buffer overflows don't just crash the program unless
> you have some really nifty guard pages, but overwrite other things in
> memory. This means an attacker can, in certain situations, use it to create
> something that not just crashes, but with a nifty input create an exploit.
> Having a 'Oh it will just crash' attitude is 'not the best'.
>
> 2018-04-02 19:41 GMT+02:00 Eric Kohl <[e-mail]:[e-mail]>:
> > Hello Thomas,
> >
> > you're right, using the run-time size checks are a good way to keep
> > application from crashing because of buffer overflows. They'll just keep
> > on using corrupt data instead! If you want to fix this problem: Don't
> > use C! Use C++, C#, Java etc. instead!
> >
> > I prefer to see an application crash because of a buffer overflow rather
> > than seeing it store truncated phone numbers in a database.
> >
> > PS: If the timeout is longer than a day, winlogon uses the "%d days"
> > format. In the end, a buffer of 10 characters is still large enough.
> >
> > PPS: I'll keep using the old functions until you remove them from the
> > runtime code.
> >
> > Regards
> > Eric
> >
> > Am 02.04.2018 um 14:12 schrieb Thomas Faber:
> > > Hey Eric,
> > >
> > > On 2018-04-02 12:58, Eric Kohl wrote:
> > > > RtlStringCbPrintfW(strbuf, sizeof(strbuf), L"%d:%d:%d", hours,
> > > >
> > > > minutes, seconds);
> > > > swprintf(szBuffer, L"%02d:%02d:%02d", iHours, iMinutes, iSeconds);
> > >
> > > Unfortunately I must disagree with this change.
> > >
> > > Buffer overflows are a big enough threat that code review and
> > > static analysis are not generally considered sufficient to protect
> > > against them.
> > > So it's best practice for new code to always verify sizes at run-time,
> > > and never to use s(w)print.
> > >
> > > Best regards,
> > > Thomas
> > >
> > > PS: from what I see, iHours can be as large as 1193046, which won't
> > >      fit in 2 digits

_______________________________________________
Ros-dev mailing list
[e-mail]:[e-mail]
http://www.reactos.org/mailman/listinfo/ros-dev

[Maya Posch]

One thing weighs in favour of using C++ over C and that is the increased 
type checking with the former, whereas the latter is decidedly weakly-typed.

That said, C is hardly the maintenance and programming nightmare that it 
is made out to be by many. Proper design and architecture can make life 
so much easier no matter which language one uses.

Naturally, if we're going to get paranoid about languages used, then I'd 
be in favour of using Ada. If it's good enough for avionics and ICBMs, 
then it's good enough for a desktop OS :)

Maya

On 2018/04/05 14:42, Riccardo Paolo Bestetti wrote:
> Some people use the very same argument to write OS’s in Rust. It’s not 
> like it doesn’t make sense, it’s just not necessary.
>
> If C++ and Rust can compile to safe code, so can C. You just gotta 
> define and abide by sensible safety practices.
>
> </stating_the_obvious>
>
> BR,
>
> Riccardo P. Bestetti
>
> *Da:* Ros-dev [mailto:[e-mail]] *Per conto di *Huw 
> Campbell
> *Inviato:* giovedì 5 aprile 2018 13:35
> *A:* ReactOS Development List [e-mail]
> *Oggetto:* Re: [ros-dev] [ros-diffs] 01/01: [WINLOGON] Clean up part 2 - 
> Replace the UNICODE_STRING usMessage by a PWSTR pszMessage. - Use the 
> "%02d:%02d:%02d" time format and get rid of the safe string printf 
> because the string will NEVER be longer than 8 charac...
>
> Hey,
>
> I'm going to chime in here.
> If you want to fix this problem: Don't use C! Use C++, C#, Java etc.
> instead!
>
> Honestly, this is inane right? ReactOS currently uses C predominantly, 
> but we can't
>
> use that as an excuse for shitty code which breaks, crashes, or has 
> buffer overflows.
>
> Security issues are not ok.
>
> Cheers,
>
> Huw
>
> On Tue, Apr 3, 2018 at 5:53 AM, Thomas Faber <[e-mail] 
> mailto:[e-mail]> wrote:
> Finding bugs is definitely a valid concern. But there is, of course,
> a version that addresses both problems:
> NT_VERIFY(NT_SUCCESS(RtlStringCbPrintfW(...)));
>
> This will assert in case the buffer is too small, while still never
> causing an overflow.
>
> We could provide wrappers to require less typing or raise an exception
> even on release builds, if that's more desirable.
>
> Best,
> Thomas
>
>
>
>
> On 2018-04-02 20:28, Magnus Johnsson wrote:
>  > Eric, the thing is, buffer overflows don't just crash the program
> unless
>  > you have some really nifty guard pages, but overwrite other things in
>  > memory. This means an attacker can, in certain situations, use it
> to create
>  > something that not just crashes, but with a nifty input create an
> exploit.
>  > Having a 'Oh it will just crash' attitude is 'not the best'.
>  >
>  > 2018-04-02 19:41 GMT+02:00 Eric Kohl <[e-mail]
> <mailto:[e-mail]>>:
>  >
>  >> Hello Thomas,
>  >>
>  >> you're right, using the run-time size checks are a good way to keep
>  >> application from crashing because of buffer overflows. They'll
> just keep
>  >> on using corrupt data instead! If you want to fix this problem:
> Don't
>  >> use C! Use C++, C#, Java etc. instead!
>  >>
>  >> I prefer to see an application crash because of a buffer
> overflow rather
>  >> than seeing it store truncated phone numbers in a database.
>  >>
>  >> PS: If the timeout is longer than a day, winlogon uses the "%d days"
>  >> format. In the end, a buffer of 10 characters is still large enough.
>  >>
>  >> PPS: I'll keep using the old functions until you remove them
> from the
>  >> runtime code.
>  >>
>  >>
>  >> Regards
>  >> Eric
>  >>
>  >> Am 02.04.2018 um 14:12 schrieb Thomas Faber:
>  >>> Hey Eric,
>  >>>
>  >>> On 2018-04-02 12:58, Eric Kohl wrote:
>  >>>> -    RtlStringCbPrintfW(strbuf, sizeof(strbuf), L"%d:%d:%d",
> hours,
>  >>>> minutes, seconds);
>  >>>> +    swprintf(szBuffer, L"%02d:%02d:%02d", iHours, iMinutes,
> iSeconds);
>  >>>
>  >>> Unfortunately I must disagree with this change.
>  >>>
>  >>> Buffer overflows are a big enough threat that code review and
>  >>> static analysis are not generally considered sufficient to protect
>  >>> against them.
>  >>> So it's best practice for new code to always verify sizes at
> run-time,
>  >>> and never to use s(w)print.
>  >>>
>  >>> Best regards,
>  >>> Thomas
>  >>>
>  >>> PS: from what I see, iHours can be as large as 1193046, which won't
>  >>>      fit in 2 digits
>
>
> _______________________________________________
> Ros-dev mailing list
> [e-mail] <mailto:[e-mail]>
> http://www.reactos.org/mailman/listinfo/ros-dev
>
>
>
> Ros-dev mailing list
> [e-mail]
> http://www.reactos.org/mailman/listinfo/ros-dev

[Thomas Mueller]

from Maya Posch:
> One thing weighs in favour of using C++ over C and that is the increased type
> checking with the former, whereas the latter is decidedly weakly-typed.
> That said, C is hardly the maintenance and programming nightmare that it is
> made out to be by many. Proper design and architecture can make life so much
> easier no matter which language one uses.
> Naturally, if we're going to get paranoid about languages used, then I'd be in
> favour of using Ada. If it's good enough for avionics and ICBMs, then it's
> good enough for a desktop OS :)
> Maya

Idea of using C++ is not so crazy.  Haiku is programmed largely in C++.

C++ is more strongly typed than C.  I have some familiarity with Ada, which is (I believe) more strongly typed.

I am very limited in my ability to test ReactOS because of no place to put it.  Hard drives are partitioned GPT, and booting from USB stick is problematic from what I read.

Tom