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.