Re: Single PR or many smaller PRs?

[Demi M. Obenour]

On 10/15/20 2:06 PM, Chris Bennett wrote:
> On Thu, Oct 15, 2020 at 01:14:00PM -0400, Demi M. Obenour wrote:
>> On 10/15/20 12:05 PM, Joerg Jung wrote:
>>> As mentioned by Gilles earlier, please send them as unified diffs 
>>> to t...@openbsd.org  if you want to have them 
>>> reviewed.
>>
>> My branch is based on the portable branch.  Do I need to rebase off
>> of the OpenBSD repository first?
>>
>> Demi
> 
> Your diff's must come off of src for OpenBSD -current and you must also
> be running the latest and constantly moving -current.
> See the FAQ on https://www.openbsd.org

I can prepare diffs against -current, but they haven’t been tested
on OpenBSD as I don’t currently have any systems running OpenBSD.

Sincerely,

Demi


OpenPGP_0xB288B55FFF9C22C1.asc
Description: application/pgp-keys


OpenPGP_signature
Description: OpenPGP digital signature

Re: Single PR or many smaller PRs?

[Demi M. Obenour]

On 10/15/20 2:06 PM, Chris Bennett wrote:
> On Thu, Oct 15, 2020 at 01:14:00PM -0400, Demi M. Obenour wrote:
>> On 10/15/20 12:05 PM, Joerg Jung wrote:
>>> As mentioned by Gilles earlier, please send them as unified diffs 
>>> to t...@openbsd.org  if you want to have them 
>>> reviewed.
>>
>> My branch is based on the portable branch.  Do I need to rebase off
>> of the OpenBSD repository first?
>>
>> Demi
> 
> Your diff's must come off of src for OpenBSD -current and you must also
> be running the latest and constantly moving -current.
> See the FAQ on https://www.openbsd.org

I can prepare diffs against -current, but they haven’t been tested
on OpenBSD as I don’t currently have any systems running OpenBSD.

Sincerely,

Demi


OpenPGP_0xB288B55FFF9C22C1.asc
Description: application/pgp-keys


OpenPGP_signature
Description: OpenPGP digital signature

Re: Single PR or many smaller PRs?

[Chris Bennett]

On Thu, Oct 15, 2020 at 01:14:00PM -0400, Demi M. Obenour wrote:
> On 10/15/20 12:05 PM, Joerg Jung wrote:
> > As mentioned by Gilles earlier, please send them as unified diffs 
> > to t...@openbsd.org  if you want to have them 
> > reviewed.
> 
> My branch is based on the portable branch.  Do I need to rebase off
> of the OpenBSD repository first?
> 
> Demi

Your diff's must come off of src for OpenBSD -current and you must also
be running the latest and constantly moving -current.
See the FAQ on https://www.openbsd.org

Git is not relevant for this work.

Thanks for your work.

Chris Bennett

Re: Single PR or many smaller PRs?

[Demi M. Obenour]

On 10/15/20 12:05 PM, Joerg Jung wrote:
> As mentioned by Gilles earlier, please send them as unified diffs 
> to t...@openbsd.org  if you want to have them 
> reviewed.

My branch is based on the portable branch.  Do I need to rebase off
of the OpenBSD repository first?

Demi


OpenPGP_0xB288B55FFF9C22C1.asc
Description: application/pgp-keys


OpenPGP_signature
Description: OpenPGP digital signature

Re: Single PR or many smaller PRs?

[Joerg Jung]

> On 15. Oct 2020, at 16:32, Demi M. Obenour  wrote:
> 
> On 10/15/20 12:48 AM, Demi M. Obenour wrote:
>> On 10/14/20 7:31 PM, gil...@poolp.org wrote:
>>> October 14, 2020 11:31 PM, "Demi M. Obenour"  wrote:
 That makes sense. I will make a separate PR that just has some
 automake fixes.
 
>>> 
>>> perfect
>> 
>> I filed https://github.com/OpenSMTPD/OpenSMTPD/pull/1093.  That’s the
>> biggest diff in the whole series.
>> 
> I do look at the bug tracker still but will only handle portable bits
> as I no longer commit to OpenBSD.
 
 Do the basic ideas look good? Fixing type confusion bugs in imsg
 handling is definitely a good thing, but I am less sure about the rest
 of the changes. My goal is to limit the damage that can be done by
 a compromised unprivileged process, but it adds some complexity.
 
>>> 
>>> To be honest, I haven't looked at the diff yet as it was a bit too big
>>> and slowly catching up with the hundreds of mails I'm late reading.
>> 
>> Sorry about that, and thanks for the quick response.  The individual
>> commits are much easier to review separately, but that isn’t
>> necessarily obvious from the GitHub UI.
>> 
>> https://github.com/OpenSMTPD/OpenSMTPD/commit/115d6177adb008e45dd20bf716f576266237b96a
>> is a trivial fix to error handling in mail.lmtp.
>> 
>> https://github.com/OpenSMTPD/OpenSMTPD/commit/9cc9a02c2d10dd76c615ad8d228ca80c8cd68429
>> is probably the most controversial diff, and I would greatly
>> appreciate if you could review it before I send it off to tech@.
>> It avoids spawning a shell for built-in MDAs, instead using execle().
>> This is a security, performance, and correctness improvement, and
>> fixes GitHub issue #921.  The trade-off is that it adds a small
>> amount of special-case code for each built-in delivery method, and
>> skips MDA wrappers for them.  In my use-case, this is a good thing,
>> but I understand if others disagree.
>> 
>> https://github.com/OpenSMTPD/OpenSMTPD/commit/dea15ef7dad1e60a3e3c2fb3bb7d30dc8413576e
>> rejects empty maildir and LMTP paths when parsing the configuration
>> file, to avoid more confusing errors during delivery.
>> 
>> https://github.com/OpenSMTPD/OpenSMTPD/commit/f99c69644788b7d0d6a02a33884e1285347c018d
>> makes the MDA command available (via the `MDA` environment variable)
>> to MDA wrappers.  Using the %{mda} expansion quickly turned into a
>> quoting nightmare that I didn’t want to deal with.
>> 
>> https://github.com/OpenSMTPD/OpenSMTPD/commit/b10863e4a9e8d3899db182c51122e8a2aebafb3c
>> redoes getpwnam lookups in the parent process, to protect against
>> receiving spoofed values from an unprivileged (and therefore less
>> trusted) child process.
>> 
>> Finally,
>> https://github.com/OpenSMTPD/OpenSMTPD/commit/eebff03ea7226272fa45191c953772abcefff9aa
>> fixes some type confusion and shell injection bugs I found.  The type
>> confusion bugs involve imsg processing, and could only be exploited
>> if an OpenSMTPD process has already been compromised.  Therefore,
>> I don’t consider them to be security holes.  The shell injection
>> bugs aren’t exploitable by an attacker, but they do cause incorrect
>> behavior in certain cases.
>> 
>> Thank you so much for your time and effort.  I have been very
>> pleasantly surprised by your quick and thorough responses.
>> 
>> Sincerely,
>> 
>> Demi
> 
> The above URLs have the wrong repository name, sorry.  The correct
> repository is https://github.com/DemiMarie/OpenSMTPD.

As mentioned by Gilles earlier, please send them as unified diffs 
to t...@openbsd.org  if you want to have them reviewed.

Re: Single PR or many smaller PRs?

[Demi M. Obenour]

On 10/15/20 12:48 AM, Demi M. Obenour wrote:
> On 10/14/20 7:31 PM, gil...@poolp.org wrote:
>> October 14, 2020 11:31 PM, "Demi M. Obenour"  wrote:
>>> That makes sense. I will make a separate PR that just has some
>>> automake fixes.
>>>
>>
>> perfect
> 
> I filed https://github.com/OpenSMTPD/OpenSMTPD/pull/1093.  That’s the
> biggest diff in the whole series.
>  
 I do look at the bug tracker still but will only handle portable bits
 as I no longer commit to OpenBSD.
>>>
>>> Do the basic ideas look good? Fixing type confusion bugs in imsg
>>> handling is definitely a good thing, but I am less sure about the rest
>>> of the changes. My goal is to limit the damage that can be done by
>>> a compromised unprivileged process, but it adds some complexity.
>>>
>>
>> To be honest, I haven't looked at the diff yet as it was a bit too big
>> and slowly catching up with the hundreds of mails I'm late reading.
> 
> Sorry about that, and thanks for the quick response.  The individual
> commits are much easier to review separately, but that isn’t
> necessarily obvious from the GitHub UI.
> 
> https://github.com/OpenSMTPD/OpenSMTPD/commit/115d6177adb008e45dd20bf716f576266237b96a
> is a trivial fix to error handling in mail.lmtp.
> 
> https://github.com/OpenSMTPD/OpenSMTPD/commit/9cc9a02c2d10dd76c615ad8d228ca80c8cd68429
> is probably the most controversial diff, and I would greatly
> appreciate if you could review it before I send it off to tech@.
> It avoids spawning a shell for built-in MDAs, instead using execle().
> This is a security, performance, and correctness improvement, and
> fixes GitHub issue #921.  The trade-off is that it adds a small
> amount of special-case code for each built-in delivery method, and
> skips MDA wrappers for them.  In my use-case, this is a good thing,
> but I understand if others disagree.
> 
> https://github.com/OpenSMTPD/OpenSMTPD/commit/dea15ef7dad1e60a3e3c2fb3bb7d30dc8413576e
> rejects empty maildir and LMTP paths when parsing the configuration
> file, to avoid more confusing errors during delivery.
> 
> https://github.com/OpenSMTPD/OpenSMTPD/commit/f99c69644788b7d0d6a02a33884e1285347c018d
> makes the MDA command available (via the `MDA` environment variable)
> to MDA wrappers.  Using the %{mda} expansion quickly turned into a
> quoting nightmare that I didn’t want to deal with.
> 
> https://github.com/OpenSMTPD/OpenSMTPD/commit/b10863e4a9e8d3899db182c51122e8a2aebafb3c
> redoes getpwnam lookups in the parent process, to protect against
> receiving spoofed values from an unprivileged (and therefore less
> trusted) child process.
> 
> Finally,
> https://github.com/OpenSMTPD/OpenSMTPD/commit/eebff03ea7226272fa45191c953772abcefff9aa
> fixes some type confusion and shell injection bugs I found.  The type
> confusion bugs involve imsg processing, and could only be exploited
> if an OpenSMTPD process has already been compromised.  Therefore,
> I don’t consider them to be security holes.  The shell injection
> bugs aren’t exploitable by an attacker, but they do cause incorrect
> behavior in certain cases.
> 
> Thank you so much for your time and effort.  I have been very
> pleasantly surprised by your quick and thorough responses.
> 
> Sincerely,
> 
> Demi

The above URLs have the wrong repository name, sorry.  The correct
repository is https://github.com/DemiMarie/OpenSMTPD.

Sincerely,

Demi



OpenPGP_0xB288B55FFF9C22C1.asc
Description: application/pgp-keys


OpenPGP_signature
Description: OpenPGP digital signature

Re: Single PR or many smaller PRs?

[Demi M. Obenour]

On 10/14/20 7:31 PM, gil...@poolp.org wrote:
> October 14, 2020 11:31 PM, "Demi M. Obenour"  wrote:
>> That makes sense. I will make a separate PR that just has some
>> automake fixes.
>>
> 
> perfect

I filed https://github.com/OpenSMTPD/OpenSMTPD/pull/1093.  That’s the
biggest diff in the whole series.
 
>>> I do look at the bug tracker still but will only handle portable bits
>>> as I no longer commit to OpenBSD.
>>
>> Do the basic ideas look good? Fixing type confusion bugs in imsg
>> handling is definitely a good thing, but I am less sure about the rest
>> of the changes. My goal is to limit the damage that can be done by
>> a compromised unprivileged process, but it adds some complexity.
>>
> 
> To be honest, I haven't looked at the diff yet as it was a bit too big
> and slowly catching up with the hundreds of mails I'm late reading.

Sorry about that, and thanks for the quick response.  The individual
commits are much easier to review separately, but that isn’t
necessarily obvious from the GitHub UI.

https://github.com/OpenSMTPD/OpenSMTPD/commit/115d6177adb008e45dd20bf716f576266237b96a
is a trivial fix to error handling in mail.lmtp.

https://github.com/OpenSMTPD/OpenSMTPD/commit/9cc9a02c2d10dd76c615ad8d228ca80c8cd68429
is probably the most controversial diff, and I would greatly
appreciate if you could review it before I send it off to tech@.
It avoids spawning a shell for built-in MDAs, instead using execle().
This is a security, performance, and correctness improvement, and
fixes GitHub issue #921.  The trade-off is that it adds a small
amount of special-case code for each built-in delivery method, and
skips MDA wrappers for them.  In my use-case, this is a good thing,
but I understand if others disagree.

https://github.com/OpenSMTPD/OpenSMTPD/commit/dea15ef7dad1e60a3e3c2fb3bb7d30dc8413576e
rejects empty maildir and LMTP paths when parsing the configuration
file, to avoid more confusing errors during delivery.

https://github.com/OpenSMTPD/OpenSMTPD/commit/f99c69644788b7d0d6a02a33884e1285347c018d
makes the MDA command available (via the `MDA` environment variable)
to MDA wrappers.  Using the %{mda} expansion quickly turned into a
quoting nightmare that I didn’t want to deal with.

https://github.com/OpenSMTPD/OpenSMTPD/commit/b10863e4a9e8d3899db182c51122e8a2aebafb3c
redoes getpwnam lookups in the parent process, to protect against
receiving spoofed values from an unprivileged (and therefore less
trusted) child process.

Finally,
https://github.com/OpenSMTPD/OpenSMTPD/commit/eebff03ea7226272fa45191c953772abcefff9aa
fixes some type confusion and shell injection bugs I found.  The type
confusion bugs involve imsg processing, and could only be exploited
if an OpenSMTPD process has already been compromised.  Therefore,
I don’t consider them to be security holes.  The shell injection
bugs aren’t exploitable by an attacker, but they do cause incorrect
behavior in certain cases.

Thank you so much for your time and effort.  I have been very
pleasantly surprised by your quick and thorough responses.

Sincerely,

Demi


OpenPGP_0xB288B55FFF9C22C1.asc
Description: application/pgp-keys


OpenPGP_signature
Description: OpenPGP digital signature

Re: Single PR or many smaller PRs?

[gilles]

October 14, 2020 11:31 PM, "Demi M. Obenour"  wrote:

> On 10/14/20 3:18 PM, gil...@poolp.org wrote:
> 
>> October 12, 2020 10:06 PM, "Demi M. Obenour"  wrote:
>> 
>>> I created https://github.com/OpenSMTPD/OpenSMTPD/pull/1087, which
>>> fixes some bugs and avoids spawning shells when it isn’t necessary
>>> to do so. Should I split it up into multiple smaller PRs? Also,
>>> it has only been tested on Linux.
>> 
>> Hello,
>> 
>> First of all, you should definitely split diffs into multiple ones so
>> they can be evaluated separately otherwise they are too big to review
>> and can only be rejected as a whole. I personally can't spend time to
>> review big diffs at the moment while I could probably review a lot of
>> smaller ones.
>> 
>> You should also send the diffs to OpenBSD tech@ list because Github's
>> commits and pull requests are only done for portability purposes. The
>> official repository for OpenSMTPD is OpenBSD. The only things that'll
>> be merged from Github pull requests are ones that make changes out of
>> smtpd/ subdirectory, or inside smtpd/ subdirectory when it fixes some
>> build issues on other systems. Any other change should go through the
>> OpenBSD developers via diffs sent to tech@.
> 
> That makes sense. I will make a separate PR that just has some
> automake fixes.
> 

perfect


>> I do look at the bug tracker still but will only handle portable bits
>> as I no longer commit to OpenBSD.
> 
> Do the basic ideas look good? Fixing type confusion bugs in imsg
> handling is definitely a good thing, but I am less sure about the rest
> of the changes. My goal is to limit the damage that can be done by
> a compromised unprivileged process, but it adds some complexity.
> 

To be honest, I haven't looked at the diff yet as it was a bit too big
and slowly catching up with the hundreds of mails I'm late reading.


> Specifically, my changes create a distinction between delivery
> methods specified in smtpd.conf and those specified in aliases or
> forward files. In the first case, my changes cause OpenSMTPD to avoid
> spawning a shell, instead directly executing the command with execle().
> This is faster, but means that MDA wrappers no longer run. In the
> second case, however, MDA wrappers will continue to run. I plan on
> then using a custom MDA wrapper to ensure ensure that only allowlisted
> MDAs can run. To make this safe, I also make PROC_PARENT re-lookup
> user information using getpwnam().
> 

I can't really comment without seeing the diff because reading this
paragraph raises many questions and worries which would be answered
reading the diff.


> Postfix takes a different approach ― its local(8) service is called
> once for each user that it needs to deliver to. It reads ~/.forward
> files and then executes commands accordingly. I prefer this approach,
> but it is a much larger refactor.
>

Re: Single PR or many smaller PRs?

[Demi M. Obenour]

On 10/14/20 3:18 PM, gil...@poolp.org wrote:
> October 12, 2020 10:06 PM, "Demi M. Obenour"  wrote:
> 
>> I created https://github.com/OpenSMTPD/OpenSMTPD/pull/1087, which
>> fixes some bugs and avoids spawning shells when it isn’t necessary
>> to do so. Should I split it up into multiple smaller PRs? Also,
>> it has only been tested on Linux.
>>
> 
> Hello,
> 
> First of all, you should definitely split diffs into multiple ones so
> they can be evaluated separately otherwise they are too big to review
> and can only be rejected as a whole. I personally can't spend time to
> review big diffs at the moment while I could probably review a lot of
> smaller ones.
> 
> You should also send the diffs to OpenBSD tech@ list because Github's
> commits and pull requests are only done for portability purposes. The
> official repository for OpenSMTPD is OpenBSD. The only things that'll
> be merged from Github pull requests are ones that make changes out of
> smtpd/ subdirectory, or inside smtpd/ subdirectory when it fixes some
> build issues on other systems. Any other change should go through the
> OpenBSD developers via diffs sent to tech@.

That makes sense.  I will make a separate PR that just has some
automake fixes.

> I do look at the bug tracker still but will only handle portable bits
> as I no longer commit to OpenBSD.

Do the basic ideas look good?  Fixing type confusion bugs in imsg
handling is definitely a good thing, but I am less sure about the rest
of the changes.  My goal is to limit the damage that can be done by
a compromised unprivileged process, but it adds some complexity.

Specifically, my changes create a distinction between delivery
methods specified in smtpd.conf and those specified in aliases or
forward files.  In the first case, my changes cause OpenSMTPD to avoid
spawning a shell, instead directly executing the command with execle().
This is faster, but means that MDA wrappers no longer run.  In the
second case, however, MDA wrappers will continue to run.  I plan on
then using a custom MDA wrapper to ensure ensure that only allowlisted
MDAs can run.  To make this safe, I also make PROC_PARENT re-lookup
user information using getpwnam().

Postfix takes a different approach ― its local(8) service is called
once for each user that it needs to deliver to.  It reads ~/.forward
files and then executes commands accordingly.  I prefer this approach,
but it is a much larger refactor.

> Gilles

Demi



OpenPGP_0xB288B55FFF9C22C1.asc
Description: application/pgp-keys


OpenPGP_signature
Description: OpenPGP digital signature

Re: Single PR or many smaller PRs?

[gilles]

October 12, 2020 10:06 PM, "Demi M. Obenour"  wrote:

> I created https://github.com/OpenSMTPD/OpenSMTPD/pull/1087, which
> fixes some bugs and avoids spawning shells when it isn’t necessary
> to do so. Should I split it up into multiple smaller PRs? Also,
> it has only been tested on Linux.
> 

Hello,

First of all, you should definitely split diffs into multiple ones so
they can be evaluated separately otherwise they are too big to review
and can only be rejected as a whole. I personally can't spend time to
review big diffs at the moment while I could probably review a lot of
smaller ones.

You should also send the diffs to OpenBSD tech@ list because Github's
commits and pull requests are only done for portability purposes. The
official repository for OpenSMTPD is OpenBSD. The only things that'll
be merged from Github pull requests are ones that make changes out of
smtpd/ subdirectory, or inside smtpd/ subdirectory when it fixes some
build issues on other systems. Any other change should go through the
OpenBSD developers via diffs sent to tech@.

I do look at the bug tracker still but will only handle portable bits
as I no longer commit to OpenBSD.

Gilles

Single PR or many smaller PRs?

[Demi M. Obenour]

I created https://github.com/OpenSMTPD/OpenSMTPD/pull/1087, which
fixes some bugs and avoids spawning shells when it isn’t necessary
to do so.  Should I split it up into multiple smaller PRs?  Also,
it has only been tested on Linux.

Sincerely,

Demi


OpenPGP_0xB288B55FFF9C22C1.asc
Description: application/pgp-keys


OpenPGP_signature
Description: OpenPGP digital signature