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