-----Original Message-----
From: rfc@edk2.groups.io <rfc@edk2.groups.io> On Behalf Of Simo Sorce via
groups.io
Sent: Friday, March 19, 2021 9:26 AM
To: Laszlo Ersek <lersek@...>; edk2-rfc-groups-io
<rfc@edk2.groups.io>
Cc: Maciej Rabeda <maciej.rabeda@...>; Jiaxin Wu
<jiaxin.wu@...>; Siyuan Fu <siyuan.fu@...>; Daniel P. Berrangé
<berrange@...>; Yash Mankad <ymankad@...>
Subject: Re: [edk2-rfc] removing CHAP-MD5 from IScsiDxe

On Fri, 2021-03-19 at 14:15 +0100, Laszlo Ersek wrote:

Hi,

RFC 7143 requires CHAP-MD5 as a mandatory option offered by an iscsi
client (initiator).

At the same time, edk2 has deprecated MD5 in general, as a
cryptographically weak hash algorithm.

Consequently, the "NetworkDefines.dsc.inc" file defines
NETWORK_ISCSI_ENABLE with default value FALSE (commit 4ecb1ba5efa3 /
TianoCore#3003). Platforms that want to include IScsiDxe need to opt
in consciously to the presence of CHAP-MD5 in the code.

We're not happy with this granularity. We'd prefer:

- explicitly breaking RFC 7143 conformance,
- removing CHAP-MD5,
- and using an IScsiDxe variant that is honest about having no
confidentiality / integrity.

IScsiDxe is safe on a trusted network, and only on a trusted network.
The presence of CHAP-MD5 suggests it may be safe on an untrusted
network too, and that implication (not the whole iscsi client
functionality) is what we should rid ourselves of.

Lazlo,
This sounds like the right direction to me.

My 2C,
Simo.

Are NetworkPkg maintainers open to breaking RFC 7143 conformance in
IScsiDxe (perhaps with a feature PCD?), or should we look into this
only downstream?

Downstream, we might decide to drop IScsiDxe altogether, in sync with
the upstream NETWORK_ISCSI_ENABLE=FALSE default -- that decision has
not been made yet. Now I'm just testing whether keeping IScsiDxe
enabled down-stream would require us to carry downstream-only patches.

Thanks
Laszlo

--
Simo Sorce
RHEL Crypto Team
Red Hat, Inc

-----Original Message-----
From: rfc@edk2.groups.io <rfc@edk2.groups.io> On Behalf Of Samer El-Haj-
Mahmoud
Sent: Friday, March 19, 2021 10:08 AM
To: rfc@edk2.groups.io; simo@...; Laszlo Ersek
<lersek@...>
Cc: Maciej Rabeda <maciej.rabeda@...>; Wu, Jiaxin
<jiaxin.wu@...>; Fu, Siyuan <siyuan.fu@...>; Daniel P.
Berrangé <berrange@...>; Yash Mankad <ymankad@...>;
Pete Batard <pete@...>; Samer El-Haj-Mahmoud <Samer.El-Haj-
Mahmoud@...>
Subject: Re: [edk2-rfc] removing CHAP-MD5 from IScsiDxe

When the RPi UEFI FW dropped iSCSI (because it was removed from
NetworkDefines.dsc.inc), we got multiple users complaining since they had
existing use cases that depended on that. See for instance the RPi4 UEFI
reported bug: https://github.com/pftf/RPi4/issues/125

Some use-cases and user scenarios for iSCSI on RPi4:
https://blogs.vmware.com/arm/2020/10/17/esxi-arm-with-iscsi/
https://tech.xlab.si/blog/pxe-boot-raspberry-pi-iscsi/
https://www.virtuallyghetto.com/2020/07/two-methods-to-network-boot-
raspberry-pi-4.html

The RPi4 users preferred having iSCSI (without CHAP) than not having it at all.
We resorted to enabling it out-of-tree using a build switch.

From my experience, I think the EDK2 iScsiDxe software initiator is being
wildly used on many servers (and OSes) across the industry (just google
"UEFI iSCSI"). Having it silently dropped from EDK2 is not optimal. I like
Laszlo's idea of a PCD to break the RFC (and using an iSCSI boot variant that
does not support CHAP), and I think it is a good compromise. The alternative
is for downstream implementations to resort to enabling it (probably just set
NETWORK_ISCSI_ENABLE=TRUE) to keep their users happy.

Thanks,
--Samer

-----Original Message-----
From: rfc@edk2.groups.io <rfc@edk2.groups.io> On Behalf Of Simo Sorce
via groups.io
Sent: Friday, March 19, 2021 9:26 AM
To: Laszlo Ersek <lersek@...>; edk2-rfc-groups-io
<rfc@edk2.groups.io>
Cc: Maciej Rabeda <maciej.rabeda@...>; Jiaxin Wu
<jiaxin.wu@...>; Siyuan Fu <siyuan.fu@...>; Daniel P.
Berrangé <berrange@...>; Yash Mankad

<ymankad@...>

Subject: Re: [edk2-rfc] removing CHAP-MD5 from IScsiDxe

On Fri, 2021-03-19 at 14:15 +0100, Laszlo Ersek wrote:

Hi,

RFC 7143 requires CHAP-MD5 as a mandatory option offered by an iscsi
client (initiator).

At the same time, edk2 has deprecated MD5 in general, as a
cryptographically weak hash algorithm.

Consequently, the "NetworkDefines.dsc.inc" file defines
NETWORK_ISCSI_ENABLE with default value FALSE (commit

4ecb1ba5efa3 /

TianoCore#3003). Platforms that want to include IScsiDxe need to opt
in consciously to the presence of CHAP-MD5 in the code.

We're not happy with this granularity. We'd prefer:

- explicitly breaking RFC 7143 conformance,
- removing CHAP-MD5,
- and using an IScsiDxe variant that is honest about having no
confidentiality / integrity.

IScsiDxe is safe on a trusted network, and only on a trusted network.
The presence of CHAP-MD5 suggests it may be safe on an untrusted
network too, and that implication (not the whole iscsi client
functionality) is what we should rid ourselves of.

Lazlo,
This sounds like the right direction to me.

My 2C,
Simo.

Are NetworkPkg maintainers open to breaking RFC 7143 conformance in
IScsiDxe (perhaps with a feature PCD?), or should we look into this
only downstream?

Downstream, we might decide to drop IScsiDxe altogether, in sync
with the upstream NETWORK_ISCSI_ENABLE=FALSE default -- that
decision has not been made yet. Now I'm just testing whether keeping
IScsiDxe enabled down-stream would require us to carry downstream-

only patches.

Thanks
Laszlo

--
Simo Sorce
RHEL Crypto Team
Red Hat, Inc

IMPORTANT NOTICE: The contents of this email and any attachments are
confidential and may also be privileged. If you are not the intended recipient,
please notify the sender immediately and do not disclose the contents to any
other person, use it for any purpose, or store or copy the information in any
medium. Thank you.

I'm honestly surprised that the iSCSI RFC has not been updated to drop CHAP-MD5 at this point. I realize that is probably a bigger effort but it seems like something the industry should do. I agree that having a FeaturePcd for CHAP MD5 and having it default to False is probably the right thing to do.

-----Original Message-----
From: rfc@edk2.groups.io <rfc@edk2.groups.io> On Behalf Of Samer El-Haj-
Mahmoud
Sent: Friday, March 19, 2021 10:08 AM
To: rfc@edk2.groups.io; simo@...; Laszlo Ersek
<lersek@...>
Cc: Maciej Rabeda <maciej.rabeda@...>; Wu, Jiaxin
<jiaxin.wu@...>; Fu, Siyuan <siyuan.fu@...>; Daniel P.
Berrangé <berrange@...>; Yash Mankad <ymankad@...>;
Pete Batard <pete@...>; Samer El-Haj-Mahmoud <Samer.El-Haj-
Mahmoud@...>
Subject: Re: [edk2-rfc] removing CHAP-MD5 from IScsiDxe

When the RPi UEFI FW dropped iSCSI (because it was removed from
NetworkDefines.dsc.inc), we got multiple users complaining since they had
existing use cases that depended on that. See for instance the RPi4 UEFI
reported bug: https://github.com/pftf/RPi4/issues/125

Some use-cases and user scenarios for iSCSI on RPi4:
https://blogs.vmware.com/arm/2020/10/17/esxi-arm-with-iscsi/
https://tech.xlab.si/blog/pxe-boot-raspberry-pi-iscsi/
https://www.virtuallyghetto.com/2020/07/two-methods-to-network-boot-
raspberry-pi-4.html

The RPi4 users preferred having iSCSI (without CHAP) than not having it at all.
We resorted to enabling it out-of-tree using a build switch.

From my experience, I think the EDK2 iScsiDxe software initiator is being
wildly used on many servers (and OSes) across the industry (just google
"UEFI iSCSI"). Having it silently dropped from EDK2 is not optimal. I like
Laszlo's idea of a PCD to break the RFC (and using an iSCSI boot variant that
does not support CHAP), and I think it is a good compromise. The alternative
is for downstream implementations to resort to enabling it (probably just set
NETWORK_ISCSI_ENABLE=TRUE) to keep their users happy.

Thanks,
--Samer

-----Original Message-----
From: rfc@edk2.groups.io <rfc@edk2.groups.io> On Behalf Of Simo Sorce
via groups.io
Sent: Friday, March 19, 2021 9:26 AM
To: Laszlo Ersek <lersek@...>; edk2-rfc-groups-io
<rfc@edk2.groups.io>
Cc: Maciej Rabeda <maciej.rabeda@...>; Jiaxin Wu
<jiaxin.wu@...>; Siyuan Fu <siyuan.fu@...>; Daniel P.
Berrangé <berrange@...>; Yash Mankad

<ymankad@...>

Subject: Re: [edk2-rfc] removing CHAP-MD5 from IScsiDxe

On Fri, 2021-03-19 at 14:15 +0100, Laszlo Ersek wrote:

Hi,

RFC 7143 requires CHAP-MD5 as a mandatory option offered by an iscsi
client (initiator).

At the same time, edk2 has deprecated MD5 in general, as a
cryptographically weak hash algorithm.

Consequently, the "NetworkDefines.dsc.inc" file defines
NETWORK_ISCSI_ENABLE with default value FALSE (commit

4ecb1ba5efa3 /

TianoCore#3003). Platforms that want to include IScsiDxe need to opt
in consciously to the presence of CHAP-MD5 in the code.

We're not happy with this granularity. We'd prefer:

- explicitly breaking RFC 7143 conformance,
- removing CHAP-MD5,
- and using an IScsiDxe variant that is honest about having no
confidentiality / integrity.

IScsiDxe is safe on a trusted network, and only on a trusted network.
The presence of CHAP-MD5 suggests it may be safe on an untrusted
network too, and that implication (not the whole iscsi client
functionality) is what we should rid ourselves of.

Lazlo,
This sounds like the right direction to me.

My 2C,
Simo.

Are NetworkPkg maintainers open to breaking RFC 7143 conformance in
IScsiDxe (perhaps with a feature PCD?), or should we look into this
only downstream?

Downstream, we might decide to drop IScsiDxe altogether, in sync
with the upstream NETWORK_ISCSI_ENABLE=FALSE default -- that
decision has not been made yet. Now I'm just testing whether keeping
IScsiDxe enabled down-stream would require us to carry downstream-

only patches.

Thanks
Laszlo

--
Simo Sorce
RHEL Crypto Team
Red Hat, Inc

IMPORTANT NOTICE: The contents of this email and any attachments are
confidential and may also be privileged. If you are not the intended recipient,
please notify the sender immediately and do not disclose the contents to any
other person, use it for any purpose, or store or copy the information in any
medium. Thank you.

On Thu, Apr 01, 2021 at 04:24:27PM +0200, Rabeda, Maciej wrote:

Hi,

Sorry for the very late response.

Dropping iSCSI overall is a no-go - too many users + this is the only remote
block I/O we seem to support in EDKII.
As for RFC compliance vs EDKII policy on MD5... Naturally, CHAP with MD5
does not bring any security features due to MD5's vulnerability.
However, since MD5 is the only hash algorithm for CHAP supported by
IScsiDxe, removing MD5 implies removing CHAP-related code from IScsiDxe
overall, which I would be pretty hesitant to do.

RFC states that MD5 has to be supported, though I can see that CHAP
algorithm allows for different hash algorithms (https://www.iana.org/assignments/ppp-numbers/ppp-numbers.xhtml#ppp-numbers-9).
We could support CHAP with SHA-x in IScsiDxe, which removes the MD5
dependency and keeps the CHAP-related code in iSCSI still in place.

The question is: do OS-based initiators support hash algorithms other than
MD5 for CHAP?
I am pretty sure RHEL does (controlled via /etc/iscsi/iscsid.conf), but I am
not sure about others: Windows, VMware, ...

Linux kernel gained support for the SHA* family of hashes:

commit a572d24af4d16e70743feb0b4decb17aaae7ce43
Author: Maurizio Lombardi <mlombard@...>
Date: Mon Oct 28 13:38:20 2019 +0100

scsi: target: iscsi: CHAP: add support for SHA1, SHA256 and SHA3-256
This patch modifies the chap_server_compute_hash() function to make it
agnostic to the choice of hash algorithm that is used. It also adds
support to three new hash algorithms: SHA1, SHA256 and SHA3-256.
The chap_got_response() function has been removed because the digest type
validity is already checked by chap_server_open()
Link: https://lore.kernel.org/r/20191028123822.5864-2-mlombard@redhat.com
Signed-off-by: Maurizio Lombardi <mlombard@...>
Tested-by: Chris Leech <cleech@...>
Signed-off-by: Martin K. Petersen <martin.petersen@...>

NB SHA1 is just as undesirable as MD5 these days, so only the other two
are especially interesting/useful.

Regards,
Daniel

Maciej,

On 04/01/21 16:24, Maciej Rabeda wrote:

Hi,

Sorry for the very late response.

Dropping iSCSI overall is a no-go - too many users + this is the only
remote block I/O we seem to support in EDKII.
As for RFC compliance vs EDKII policy on MD5... Naturally, CHAP with MD5
does not bring any security features due to MD5's vulnerability.
However, since MD5 is the only hash algorithm for CHAP supported by
IScsiDxe, removing MD5 implies removing CHAP-related code from IScsiDxe
overall, which I would be pretty hesitant to do.

RFC states that MD5 has to be supported, though I can see that CHAP
algorithm allows for different hash algorithms
(https://www.iana.org/assignments/ppp-numbers/ppp-numbers.xhtml#ppp-numbers-9).

We could support CHAP with SHA-x in IScsiDxe, which removes the MD5
dependency and keeps the CHAP-related code in iSCSI still in place.

The question is: do OS-based initiators support hash algorithms other
than MD5 for CHAP?
I am pretty sure RHEL does (controlled via /etc/iscsi/iscsid.conf), but
I am not sure about others: Windows, VMware, ...

Looking at the code for a few hours, I've had the following (roundabout)
thought process.

(1) The IScsiCHAP.[hc] files hardwire the MD5 dependency, including
direct MD5 API calls to BaseCryptLib. The idea is to generalize this to
a table of digest algos, indexed / matched by CHAP_A.

The first step would be to just turn the present MD5 support into such a
"hash algo lookup" form (with an eye towards adding a SHA256 entry to
the digest algo table later).

(2) Add a SHA256 entry to the table, using CHAP_A value 7, and using
pointers to the BaseCryptLib functions Sha256GetContextSize() and friends.

(3) Make the MD5 entry conditional on ENABLE_MD5_DEPRECATED_INTERFACES.

(4) Unfortunately, step (2) would more or less reproduce the "mHashInfo"
table we already have in "SecurityPkg/Hash2DxeCrypto/Hash2DxeCrypto.c"
(that is, in the implementation of EFI_HASH2_PROTOCOL).

Duplicating that table (especially with the function pointer typedefs,
for the members in the EFI_HASH_INFO record type) is ugly as heck.

Extracting said funcptr types is a PITA too -- they incorrectly use the
EFI_ prefix (ex. EFI_HASH_UPDATE), so simply moving them to a public
header file won't fly.

(5) Well, how about rebasing IScsiDxe from BaseCryptLib to
EFI_HASH2_PROTOCOL?

It turns out that IScsiDxe depends on BaseCryptLib -- what's more, on
CryptoPkg altogether! -- only for the MD5 APIs called in
IScsiCHAPCalculateResponse() [NetworkPkg/IScsiDxe/IScsiCHAP.c].
Therefore, the digest algo generalization should actually be started by
replacing the direct MD5 calls with EFI_HASH2_PROTOCOL usage, using the
standard GUID "gEfiHashAlgorithmMD5Guid".

(6) For this, a table would still be needed, but it would map CHAP_A
values to hash algo GUIDs *only* (so it'd be a much simpler table than
the one from (2)).

(7) However... using EFI_HASH2_PROTOCOL would break compatibility, for
two reasons.

The first reason is that our EFI_HASH2_PROTOCOL implementation does not
support MD5 at all, since commit 0a1b6d0be337
("SecurityPkg/Hash2DxeCrypto: Remove MD5 support", 2020-11-17). So
platforms could not opt in to MD5 even if they wanted to.

The second reason is that platforms may include IScsiDxe (via
NETWORK_ISCSI_ENABLE), but not Hash2DxeCrypto. On such platforms, CHAP
auth would always fail (the gBS->LocateProtocol() call for
EFI_HASH2_PROTOCOL would fail). The solution for this would be to either
(a) find all platforms in edk2 *and* edk2-platforms that expose
NETWORK_ISCSI_ENABLE, and make sure they include Hash2DxeCrypto too; or
(b) modify "NetworkComponents.dsc.inc", for including Hash2DxeCrypto
alongside IScsiDxe.

The first compat breakage does not disturb me. We've already agreed that
breaking RFC compat by not offering CHAP_A=5 is fine.

The second compat breakage does bother me: I'm very much not looking
forward to (a) extending umpteen DSC files in *edk2-platforms* for
including Hash2DxeCrypto, nor do I feel happy about (b) referring to
Hash2DxeCrypto from *SecurityPkg* in the *NetworkPkg* DSC include file.

I feel that the second compat breakage invalidates the whole
EFI_HASH2_PROTOCOL consumption idea, unfortunately.

(8) Ultimately, the simplest approach is to keep IScsiDxe self-contained
(= directly dependent on BaseCryptLib), and to *replace* MD5, as the
sole supported CHAP algo, with SHA256. IScsiDxe would remain
"single-digest", so to say; there would be no change to the control flow
anywhere.

ACK?

Thanks
Laszlo

Thanks,
Maciej

On 19-Mar-21 18:39, Desimone, Nathaniel L wrote:

I'm honestly surprised that the iSCSI RFC has not been updated to drop
CHAP-MD5 at this point. I realize that is probably a bigger effort but
it seems like something the industry should do. I agree that having a
FeaturePcd for CHAP MD5 and having it default to False is probably the
right thing to do.

-----Original Message-----
From: rfc@edk2.groups.io <rfc@edk2.groups.io> On Behalf Of Samer El-Haj-
Mahmoud
Sent: Friday, March 19, 2021 10:08 AM
To: rfc@edk2.groups.io; simo@...; Laszlo Ersek
<lersek@...>
Cc: Maciej Rabeda <maciej.rabeda@...>; Wu, Jiaxin
<jiaxin.wu@...>; Fu, Siyuan <siyuan.fu@...>; Daniel P.
Berrangé <berrange@...>; Yash Mankad <ymankad@...>;
Pete Batard <pete@...>; Samer El-Haj-Mahmoud <Samer.El-Haj-
Mahmoud@...>
Subject: Re: [edk2-rfc] removing CHAP-MD5 from IScsiDxe

When the RPi UEFI FW dropped iSCSI (because it was removed from
NetworkDefines.dsc.inc), we got multiple users complaining since they
had
existing use cases that depended on that. See for instance the RPi4 UEFI
reported bug: https://github.com/pftf/RPi4/issues/125

Some use-cases and user scenarios for iSCSI on RPi4:
https://blogs.vmware.com/arm/2020/10/17/esxi-arm-with-iscsi/
https://tech.xlab.si/blog/pxe-boot-raspberry-pi-iscsi/
https://www.virtuallyghetto.com/2020/07/two-methods-to-network-boot-
raspberry-pi-4.html

The RPi4 users preferred having iSCSI (without CHAP) than not having
it at all.
We resorted to enabling it out-of-tree using a build switch.

 From my experience, I think the EDK2 iScsiDxe software initiator is
being
wildly used on many servers (and OSes) across the industry (just google
"UEFI iSCSI"). Having it silently dropped from EDK2 is not optimal. I
like
Laszlo's idea of a PCD to break the RFC (and using an iSCSI boot
variant that
does not support CHAP), and I think it is a good compromise. The
alternative
is for downstream implementations to resort to enabling it (probably
just set
NETWORK_ISCSI_ENABLE=TRUE) to keep their users happy.

Thanks,
--Samer

-----Original Message-----
From: rfc@edk2.groups.io <rfc@edk2.groups.io> On Behalf Of Simo Sorce
via groups.io
Sent: Friday, March 19, 2021 9:26 AM
To: Laszlo Ersek <lersek@...>; edk2-rfc-groups-io
<rfc@edk2.groups.io>
Cc: Maciej Rabeda <maciej.rabeda@...>; Jiaxin Wu
<jiaxin.wu@...>; Siyuan Fu <siyuan.fu@...>; Daniel P.
Berrangé <berrange@...>; Yash Mankad

<ymankad@...>

Subject: Re: [edk2-rfc] removing CHAP-MD5 from IScsiDxe

On Fri, 2021-03-19 at 14:15 +0100, Laszlo Ersek wrote:

Hi,

RFC 7143 requires CHAP-MD5 as a mandatory option offered by an iscsi
client (initiator).

At the same time, edk2 has deprecated MD5 in general, as a
cryptographically weak hash algorithm.

Consequently, the "NetworkDefines.dsc.inc" file defines
NETWORK_ISCSI_ENABLE with default value FALSE (commit

4ecb1ba5efa3 /

TianoCore#3003). Platforms that want to include IScsiDxe need to opt
in consciously to the presence of CHAP-MD5 in the code.

We're not happy with this granularity. We'd prefer:

- explicitly breaking RFC 7143 conformance,
- removing CHAP-MD5,
- and using an IScsiDxe variant that is honest about having no
confidentiality / integrity.

IScsiDxe is safe on a trusted network, and only on a trusted network.
The presence of CHAP-MD5 suggests it may be safe on an untrusted
network too, and that implication (not the whole iscsi client
functionality) is what we should rid ourselves of.

Lazlo,
This sounds like the right direction to me.

My 2C,
Simo.

Are NetworkPkg maintainers open to breaking RFC 7143 conformance in
IScsiDxe (perhaps with a feature PCD?), or should we look into this
only downstream?

Downstream, we might decide to drop IScsiDxe altogether, in sync
with the upstream NETWORK_ISCSI_ENABLE=FALSE default -- that
decision has not been made yet. Now I'm just testing whether keeping
IScsiDxe enabled down-stream would require us to carry downstream-

only patches.

Thanks
Laszlo

--
Simo Sorce
RHEL Crypto Team
Red Hat, Inc

IMPORTANT NOTICE: The contents of this email and any attachments are
confidential and may also be privileged. If you are not the intended
recipient,
please notify the sender immediately and do not disclose the contents
to any
other person, use it for any purpose, or store or copy the
information in any
medium. Thank you.

On 04/12/21 14:36, Rabeda, Maciej wrote:

Laszlo,

I do appreciate analysis - thorough as always :)

To points:

 * 1, 2, 3 - That would be inevitable in order to properly introduce
   other hash algorithms for CHAP. Adding multiple CHAP hash algorithms
   opens more questions:
     o Should CHAP algorithm be negotiated between initiator and target
       automatically or should the user choose exactly which hash
       algorithm should be used.
     o First option requires some extra logic, but does not affect
       iSCSI attempt variable structure.
     o Second option touches iSCSI attempt variable structure (selected
       algorithm has to be conveyed with the attempt), which opens up a
       problem of dealing with already defined attempts after iSCSI
       driver update.
 * 4, 5, 6, 7 - I do agree with you, both options seem to be ugly.
   NetworkPkg is self-contained and adding drivers from other packages
   is not a good idea.
 * 8 - A possibility would be to probe for EFI_HASH2_PROTOCOL. If it
   was not found, iSCSI might not expose CHAP options via HII and
   assume that CHAP is disabled.
     o Open again: how to deal with already defined attempts configured
       to use CHAP?

As for:

The first compat breakage does not disturb me. We've already agreed that
breaking RFC compat by not offering CHAP_A=5 is fine.


Is the decision on actually removing MD5 from EDKII a UEFI-wide decision
or are we establishing it locally?

Considering the origin of the idea, it comes from the Red Hat crypto
team, with regard to edk2 virtual firmware included RHEL9. So the origin
is as local as it gets. In turn, in order to avoid downstream-only
patches as much as possible ("upstream first"), we're trying to make
this change in upstream edk2, considering especially that MD5 has been
removed already from other parts of edk2. Taking it as far as UEFI spec
changes or iSCSI RFC changes, that's a bit too much for me, given the
huge delays and red tape it would mean.

In the latter case, I am leaning towards MD5 opt-out with
ENABLE_MD5_DEPRECATED_INTERFACES unless we have a consensus between
iSCSI target providers that they support SHA* hash algorithms.

Unfortunately, this is not something I can act on. "Consensus between
iSCSI target providers" is not tangible. People in that loosely defined
set don't care about this mailing list, so this is the same as the
standardization path (USWG and/or IETF (RFC)), which is a black hole.

The alternative, MD5 opt-out, is technically doable, but the *least
ugly* option for that, namely duplicating the "mHashInfo" table from
"SecurityPkg/Hash2DxeCrypto/Hash2DxeCrypto.c", is *still* frickin' ugly.
I'm OK to do it, but I'd like you to confirm that you're OK with it.

Can you please advise so that I can start writing code?

Thanks
Laszlo

Thanks,
Maciej

On 07-Apr-21 14:48, Laszlo Ersek wrote:

Maciej,

On 04/01/21 16:24, Maciej Rabeda wrote:

Hi,

Sorry for the very late response.

Dropping iSCSI overall is a no-go - too many users + this is the only
remote block I/O we seem to support in EDKII.
As for RFC compliance vs EDKII policy on MD5... Naturally, CHAP with MD5
does not bring any security features due to MD5's vulnerability.
However, since MD5 is the only hash algorithm for CHAP supported by
IScsiDxe, removing MD5 implies removing CHAP-related code from IScsiDxe
overall, which I would be pretty hesitant to do.

RFC states that MD5 has to be supported, though I can see that CHAP
algorithm allows for different hash algorithms
(https://www.iana.org/assignments/ppp-numbers/ppp-numbers.xhtml#ppp-numbers-9).


We could support CHAP with SHA-x in IScsiDxe, which removes the MD5
dependency and keeps the CHAP-related code in iSCSI still in place.

The question is: do OS-based initiators support hash algorithms other
than MD5 for CHAP?
I am pretty sure RHEL does (controlled via /etc/iscsi/iscsid.conf), but
I am not sure about others: Windows, VMware, ...

Looking at the code for a few hours, I've had the following (roundabout)
thought process.

(1) The IScsiCHAP.[hc] files hardwire the MD5 dependency, including
direct MD5 API calls to BaseCryptLib. The idea is to generalize this to
a table of digest algos, indexed / matched by CHAP_A.

The first step would be to just turn the present MD5 support into such a
"hash algo lookup" form (with an eye towards adding a SHA256 entry to
the digest algo table later).

(2) Add a SHA256 entry to the table, using CHAP_A value 7, and using
pointers to the BaseCryptLib functions Sha256GetContextSize() and
friends.

(3) Make the MD5 entry conditional on ENABLE_MD5_DEPRECATED_INTERFACES.

(4) Unfortunately, step (2) would more or less reproduce the "mHashInfo"
table we already have in "SecurityPkg/Hash2DxeCrypto/Hash2DxeCrypto.c"
(that is, in the implementation of EFI_HASH2_PROTOCOL).

Duplicating that table (especially with the function pointer typedefs,
for the members in the EFI_HASH_INFO record type) is ugly as heck.

Extracting said funcptr types is a PITA too -- they incorrectly use the
EFI_ prefix (ex. EFI_HASH_UPDATE), so simply moving them to a public
header file won't fly.

(5) Well, how about rebasing IScsiDxe from BaseCryptLib to
EFI_HASH2_PROTOCOL?

It turns out that IScsiDxe depends on BaseCryptLib -- what's more, on
CryptoPkg altogether! -- only for the MD5 APIs called in
IScsiCHAPCalculateResponse() [NetworkPkg/IScsiDxe/IScsiCHAP.c].
Therefore, the digest algo generalization should actually be started by
replacing the direct MD5 calls with EFI_HASH2_PROTOCOL usage, using the
standard GUID "gEfiHashAlgorithmMD5Guid".

(6) For this, a table would still be needed, but it would map CHAP_A
values to hash algo GUIDs *only* (so it'd be a much simpler table than
the one from (2)).

(7) However... using EFI_HASH2_PROTOCOL would break compatibility, for
two reasons.

The first reason is that our EFI_HASH2_PROTOCOL implementation does not
support MD5 at all, since commit 0a1b6d0be337
("SecurityPkg/Hash2DxeCrypto: Remove MD5 support", 2020-11-17). So
platforms could not opt in to MD5 even if they wanted to.

The second reason is that platforms may include IScsiDxe (via
NETWORK_ISCSI_ENABLE), but not Hash2DxeCrypto. On such platforms, CHAP
auth would always fail (the gBS->LocateProtocol() call for
EFI_HASH2_PROTOCOL would fail). The solution for this would be to either
(a) find all platforms in edk2 *and* edk2-platforms that expose
NETWORK_ISCSI_ENABLE, and make sure they include Hash2DxeCrypto too; or
(b) modify "NetworkComponents.dsc.inc", for including Hash2DxeCrypto
alongside IScsiDxe.

The first compat breakage does not disturb me. We've already agreed that
breaking RFC compat by not offering CHAP_A=5 is fine.

The second compat breakage does bother me: I'm very much not looking
forward to (a) extending umpteen DSC files in *edk2-platforms* for
including Hash2DxeCrypto, nor do I feel happy about (b) referring to
Hash2DxeCrypto from *SecurityPkg* in the *NetworkPkg* DSC include file.

I feel that the second compat breakage invalidates the whole
EFI_HASH2_PROTOCOL consumption idea, unfortunately.

(8) Ultimately, the simplest approach is to keep IScsiDxe self-contained
(= directly dependent on BaseCryptLib), and to *replace* MD5, as the
sole supported CHAP algo, with SHA256. IScsiDxe would remain
"single-digest", so to say; there would be no change to the control flow
anywhere.

ACK?

Thanks
Laszlo

Thanks,
Maciej

On 19-Mar-21 18:39, Desimone, Nathaniel L wrote:

I'm honestly surprised that the iSCSI RFC has not been updated to drop
CHAP-MD5 at this point. I realize that is probably a bigger effort but
it seems like something the industry should do. I agree that having a
FeaturePcd for CHAP MD5 and having it default to False is probably the
right thing to do.

-----Original Message-----
From: rfc@edk2.groups.io <rfc@edk2.groups.io> On Behalf Of Samer
El-Haj-
Mahmoud
Sent: Friday, March 19, 2021 10:08 AM
To: rfc@edk2.groups.io; simo@...; Laszlo Ersek
<lersek@...>
Cc: Maciej Rabeda <maciej.rabeda@...>; Wu, Jiaxin
<jiaxin.wu@...>; Fu, Siyuan <siyuan.fu@...>; Daniel P.
Berrangé <berrange@...>; Yash Mankad <ymankad@...>;
Pete Batard <pete@...>; Samer El-Haj-Mahmoud <Samer.El-Haj-
Mahmoud@...>
Subject: Re: [edk2-rfc] removing CHAP-MD5 from IScsiDxe

When the RPi UEFI FW dropped iSCSI (because it was removed from
NetworkDefines.dsc.inc), we got multiple users complaining since they
had
existing use cases that depended on that. See for instance the RPi4
UEFI
reported bug: https://github.com/pftf/RPi4/issues/125

Some use-cases and user scenarios for iSCSI on RPi4:
https://blogs.vmware.com/arm/2020/10/17/esxi-arm-with-iscsi/
https://tech.xlab.si/blog/pxe-boot-raspberry-pi-iscsi/
https://www.virtuallyghetto.com/2020/07/two-methods-to-network-boot-
raspberry-pi-4.html

The RPi4 users preferred having iSCSI (without CHAP) than not having
it at all.
We resorted to enabling it out-of-tree using a build switch.

  From my experience, I think the EDK2 iScsiDxe software initiator is
being
wildly used on many servers (and OSes) across the industry (just
google
"UEFI iSCSI"). Having it silently dropped from EDK2 is not optimal. I
like
Laszlo's idea of a PCD to break the RFC (and using an iSCSI boot
variant that
does not support CHAP), and I think it is a good compromise. The
alternative
is for downstream implementations to resort to enabling it (probably
just set
NETWORK_ISCSI_ENABLE=TRUE) to keep their users happy.

Thanks,
--Samer

-----Original Message-----
From: rfc@edk2.groups.io <rfc@edk2.groups.io> On Behalf Of Simo Sorce
via groups.io
Sent: Friday, March 19, 2021 9:26 AM
To: Laszlo Ersek <lersek@...>; edk2-rfc-groups-io
<rfc@edk2.groups.io>
Cc: Maciej Rabeda <maciej.rabeda@...>; Jiaxin Wu
<jiaxin.wu@...>; Siyuan Fu <siyuan.fu@...>; Daniel P.
Berrangé <berrange@...>; Yash Mankad

<ymankad@...>

Subject: Re: [edk2-rfc] removing CHAP-MD5 from IScsiDxe

On Fri, 2021-03-19 at 14:15 +0100, Laszlo Ersek wrote:

Hi,

RFC 7143 requires CHAP-MD5 as a mandatory option offered by an iscsi
client (initiator).

At the same time, edk2 has deprecated MD5 in general, as a
cryptographically weak hash algorithm.

Consequently, the "NetworkDefines.dsc.inc" file defines
NETWORK_ISCSI_ENABLE with default value FALSE (commit

4ecb1ba5efa3 /

TianoCore#3003). Platforms that want to include IScsiDxe need to opt
in consciously to the presence of CHAP-MD5 in the code.

We're not happy with this granularity. We'd prefer:

- explicitly breaking RFC 7143 conformance,
- removing CHAP-MD5,
- and using an IScsiDxe variant that is honest about having no
confidentiality / integrity.

IScsiDxe is safe on a trusted network, and only on a trusted
network.
The presence of CHAP-MD5 suggests it may be safe on an untrusted
network too, and that implication (not the whole iscsi client
functionality) is what we should rid ourselves of.

Lazlo,
This sounds like the right direction to me.

My 2C,
Simo.

Are NetworkPkg maintainers open to breaking RFC 7143 conformance in
IScsiDxe (perhaps with a feature PCD?), or should we look into this
only downstream?

Downstream, we might decide to drop IScsiDxe altogether, in sync
with the upstream NETWORK_ISCSI_ENABLE=FALSE default -- that
decision has not been made yet. Now I'm just testing whether keeping
IScsiDxe enabled down-stream would require us to carry downstream-

only patches.

Thanks
Laszlo

--
Simo Sorce
RHEL Crypto Team
Red Hat, Inc

IMPORTANT NOTICE: The contents of this email and any attachments are
confidential and may also be privileged. If you are not the intended
recipient,
please notify the sender immediately and do not disclose the contents
to any
other person, use it for any purpose, or store or copy the
information in any
medium. Thank you.