On 09/02/15 10:25, Liming Gao wrote:
> Fix one wrong offset which is passed into DES weak key checking in TdesInit().
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Qin Long <[email protected]>
> CC: Jiaxin Wu <[email protected]>
> ---
> CryptoPkg/Library/BaseCryptLib/Cipher/CryptTdes.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/CryptoPkg/Library/BaseCryptLib/Cipher/CryptTdes.c
> b/CryptoPkg/Library/BaseCryptLib/Cipher/CryptTdes.c
> index f89094a..8025a49 100644
> --- a/CryptoPkg/Library/BaseCryptLib/Cipher/CryptTdes.c
> +++ b/CryptoPkg/Library/BaseCryptLib/Cipher/CryptTdes.c
> @@ -1,9 +1,9 @@
> /** @file
> TDES Wrapper Implementation over OpenSSL.
>
> -Copyright (c) 2010 - 2012, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) 2010 - 2015, Intel Corporation. All rights reserved.<BR>
> This program and the accompanying materials
> are licensed and made available under the terms and conditions of the BSD
> License
> which accompanies this distribution. The full text of the license may be
> found at
> http://opensource.org/licenses/bsd-license.php
>
> @@ -88,22 +88,22 @@ TdesInit (
> CopyMem (KeySchedule + 1, KeySchedule, sizeof (DES_key_schedule));
> CopyMem (KeySchedule + 2, KeySchedule, sizeof (DES_key_schedule));
> return TRUE;
> }
>
> - if (DES_is_weak_key ((const_DES_cblock *) Key + 8) == 1) {
> + if (DES_is_weak_key ((const_DES_cblock *) (Key + 8)) == 1) {
> return FALSE;
> }
>
> DES_set_key_unchecked ((const_DES_cblock *) (Key + 8), KeySchedule + 1);
>
> if (KeyLength == 128) {
> CopyMem (KeySchedule + 2, KeySchedule, sizeof (DES_key_schedule));
> return TRUE;
> }
>
> - if (DES_is_weak_key ((const_DES_cblock *) Key + 16) == 1) {
> + if (DES_is_weak_key ((const_DES_cblock *) (Key + 16)) == 1) {
> return FALSE;
> }
>
> DES_set_key_unchecked ((const_DES_cblock *) (Key + 16), KeySchedule + 2);
>
>
At the risk of sounding annoying:
I hope everyone realizes that this bug used to exist in the first place
because the EDK2 coding style is *incredibly wrong* where it requires a
space character between the typecast operator and its operand.
As I've said many times (but it bears repeating), the cast operator has
one of the strongest bindings in C. Inserting the space character
between it and its operator is *actively misleading* for the programmer,
because it visually weakens the binding. (Which doesn't match reality.)
In this specific case, the original (buggy) code should have looked like
(note lack of space between operator and operand):
if (DES_is_weak_key ((const_DES_cblock *)Key + 8) == 1) {
in which form the bug is *obvious*.
Where can I file a request to remove this harmful requirement from the
EDK2 coding style? I can now provide two examples where that requirement
caused actual bugs.
Wait... I just checked the document. (Revision 0.53.) It doesn't even
talk about the cast operator! So, let me reformulate:
Where can I file a request for section "Horizontal Spacing", so that it
spells out:
Never put a space between the cast operator and its operand:
(type-name)cast-expression
The cast operator has one of the strongest bindings in the C
language. Distancing the operator from its "cast-expression"
operand in the source code visually weakens that binding, and
misleads programmers.
Although the current edk2 codebase widely violates this rule, all
newly written code must adhere to it. The previous bad practice of
inserting a space character between operator and operand resulted
in at least two actual bugs. (Fixed in patches
"OvmfPkg: Virtio drivers: fix incorrect casts in init functions"
and "CryptoPkg: Fix one wrong parameter for weak key checking".)
Thanks
Laszlo
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
