Looks good to me. I only have one small comment:
+ if (EFI_ERROR (Ikev2EncodePacket ((IKEV2_SESSION_COMMON *)
SessionCommon, IkePacket, IkeType))) {
+ return NULL;
+ }
Do we need divide it into two code lines to satisfy some compilers?
Status = Ikev2EncodePacket ((IKEV2_SESSION_COMMON *) SessionCommon, IkePacket,
IkeType));
if (EFI_ERROR (Status))
...
As long as our supported compilers will not report build errors or warnings, I
am fine with the update.
Reviewed-by: Ye Ting <[email protected]>
-----Original Message-----
From: Wu, Jiaxin
Sent: Friday, June 24, 2016 3:33 PM
To: [email protected]
Cc: Ye, Ting <[email protected]>; Fu, Siyuan <[email protected]>; Zhang, Lubo
<[email protected]>
Subject: [Patch] NetworkPkg: Avoid potential NULL pointer dereference
The commit of 6b16c9e7 removes ASSERT and use error handling in IpSecDxe
driver, but may cause the potential NULL pointer dereference. So, this patch is
used to avoid NULL pointer dereference.
Cc: Ye Ting <[email protected]>
Cc: Fu Siyuan <[email protected]>
Cc: Zhang Lubo <[email protected]>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Jiaxin Wu <[email protected]>
---
NetworkPkg/IpSecDxe/IkePacket.c | 6 +-
NetworkPkg/IpSecDxe/Ikev2/ChildSa.c | 19 ++--
NetworkPkg/IpSecDxe/Ikev2/Exchange.c | 10 ++-
NetworkPkg/IpSecDxe/Ikev2/Payload.c | 3 +
NetworkPkg/IpSecDxe/Ikev2/Sa.c | 163 ++++++++++++++++++++++++++++++++++-
5 files changed, 188 insertions(+), 13 deletions(-)
diff --git a/NetworkPkg/IpSecDxe/IkePacket.c b/NetworkPkg/IpSecDxe/IkePacket.c
index 8fd395d..d5a938e 100644
--- a/NetworkPkg/IpSecDxe/IkePacket.c
+++ b/NetworkPkg/IpSecDxe/IkePacket.c
@@ -1,9 +1,9 @@
/** @file
IKE Packet related operation.
- Copyright (c) 2010, Intel Corporation. All rights reserved.<BR>
+ Copyright (c) 2010 - 2016, 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.
@@ -201,11 +201,13 @@ IkeNetbufFromPacket (
//
// Convert Host order to Network order for IKE_PACKET header and payloads
// Encryption payloads if needed
//
if (((IKEV2_SESSION_COMMON *) SessionCommon)->IkeVer == 2) {
- Ikev2EncodePacket ((IKEV2_SESSION_COMMON *) SessionCommon, IkePacket,
IkeType);
+ if (EFI_ERROR (Ikev2EncodePacket ((IKEV2_SESSION_COMMON *)
SessionCommon, IkePacket, IkeType))) {
+ return NULL;
+ }
} else {
//
//If IKEv1 support, check it here.
//
return NULL;
diff --git a/NetworkPkg/IpSecDxe/Ikev2/ChildSa.c
b/NetworkPkg/IpSecDxe/Ikev2/ChildSa.c
index d3859e2..1f0199b 100644
--- a/NetworkPkg/IpSecDxe/Ikev2/ChildSa.c
+++ b/NetworkPkg/IpSecDxe/Ikev2/ChildSa.c
@@ -1,9 +1,9 @@
/** @file
The operations for Child SA.
- Copyright (c) 2010, Intel Corporation. All rights reserved.<BR>
+ Copyright (c) 2010 - 2016, 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.
@@ -37,22 +37,25 @@ Ikev2CreateChildGenerator (
IKEV2_CHILD_SA_SESSION *ChildSaSession;
IKEV2_SA_SESSION *IkeSaSession;
IKE_PACKET *IkePacket;
IKE_PAYLOAD *NotifyPayload;
UINT32 *MessageId;
+
+ NotifyPayload = NULL;
+ MessageId = NULL;
ChildSaSession = (IKEV2_CHILD_SA_SESSION *) SaSession;
- IkePacket = IkePacketAlloc();
- MessageId = NULL;
-
- if (IkePacket == NULL) {
+ if (ChildSaSession == NULL) {
return NULL;
}
- if (ChildSaSession == NULL) {
+
+ IkePacket = IkePacketAlloc();
+ if (IkePacket == NULL) {
return NULL;
}
+
if (Context != NULL) {
MessageId = (UINT32 *) Context;
}
IkePacket->Header->Version = (UINT8) (2 << 4);
@@ -111,10 +114,14 @@ Ikev2CreateChildGenerator (
IKEV2_NOTIFICATION_NO_ADDITIONAL_SAS,
NULL,
NULL,
0
);
+ if (NotifyPayload == NULL) {
+ IkePacketFree (IkePacket);
+ return NULL;
+ }
IKE_PACKET_APPEND_PAYLOAD (IkePacket, NotifyPayload);
//
// TODO: Support the CREATE_CHILD_SA exchange.
//
diff --git a/NetworkPkg/IpSecDxe/Ikev2/Exchange.c
b/NetworkPkg/IpSecDxe/Ikev2/Exchange.c
index 9d58ab0a..1eddbfb 100644
--- a/NetworkPkg/IpSecDxe/Ikev2/Exchange.c
+++ b/NetworkPkg/IpSecDxe/Ikev2/Exchange.c
@@ -1,9 +1,9 @@
/** @file
The general interfaces of the IKEv2.
- Copyright (c) 2010 - 2015, Intel Corporation. All rights reserved.<BR>
+ Copyright (c) 2010 - 2016, 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.
@@ -493,10 +493,14 @@ Ikev2HandleSa (
//
ASSERT (IsListEmpty (&IkeSaSession->ChildSaSessionList) &&
IsListEmpty (&IkeSaSession->ChildSaEstablishSessionList));
ChildSaSession = Ikev2ChildSaSessionCreate (IkeSaSession, UdpService);
+ if (ChildSaSession == NULL) {
+ goto ON_ERROR;
+ }
+
ChildSaCommon = &ChildSaSession->SessionCommon;
}
//
// Parse the IKE request packet according to the auth method and current
state.
@@ -517,10 +521,14 @@ Ikev2HandleSa (
//
ASSERT (IsListEmpty (&IkeSaSession->ChildSaSessionList) &&
IsListEmpty (&IkeSaSession->ChildSaEstablishSessionList));
ChildSaSession = Ikev2ChildSaSessionCreate (IkeSaSession, UdpService);
+ if (ChildSaSession == NULL) {
+ goto ON_ERROR;
+ }
+
ChildSaCommon = &ChildSaSession->SessionCommon;
//
// Initialize the SA data for Child SA.
//
diff --git a/NetworkPkg/IpSecDxe/Ikev2/Payload.c
b/NetworkPkg/IpSecDxe/Ikev2/Payload.c
index d5fe1ab..675ecf6 100644
--- a/NetworkPkg/IpSecDxe/Ikev2/Payload.c
+++ b/NetworkPkg/IpSecDxe/Ikev2/Payload.c
@@ -2556,10 +2556,13 @@ Ikev2EncodePacket (
if (SessionCommon->State >= IkeStateAuth) {
//
// Encrypt all payload and transfer IKE packet header from Host order to
Network order.
//
Status = Ikev2EncryptPacket (SessionCommon, IkePacket);
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
} else {
//
// Fill in the lenght into IkePacket header and transfer Host order to
Network order.
//
IkePacket->Header->Length = (UINT32) (sizeof (IKE_HEADER) +
IkePacket->PayloadTotalSize); diff --git a/NetworkPkg/IpSecDxe/Ikev2/Sa.c
b/NetworkPkg/IpSecDxe/Ikev2/Sa.c index c83d456..74ef79c 100644
--- a/NetworkPkg/IpSecDxe/Ikev2/Sa.c
+++ b/NetworkPkg/IpSecDxe/Ikev2/Sa.c
@@ -443,10 +443,17 @@ Ikev2AuthPskGenerator (
IkeSaSession = (IKEV2_SA_SESSION *) SaSession;
ChildSaSession = IKEV2_CHILD_SA_SESSION_BY_IKE_SA (GetFirstNode
(&IkeSaSession->ChildSaSessionList));
+ IkePacket = NULL;
+ IdPayload = NULL;
+ AuthPayload = NULL;
+ SaPayload = NULL;
+ TsiPayload = NULL;
+ TsrPayload = NULL;
+ NotifyPayload = NULL;
CpPayload = NULL;
NotifyPayload = NULL;
//
// 1. Allocate IKE Packet
@@ -486,10 +493,13 @@ Ikev2AuthPskGenerator (
//
IdPayload = Ikev2GenerateIdPayload (
&IkeSaSession->SessionCommon,
IKEV2_PAYLOAD_TYPE_AUTH
);
+ if (IdPayload == NULL) {
+ goto CheckError;
+ }
//
// 3. Generate Auth Payload
// If it is tunnel mode, should create the configuration payload after the
// Auth payload.
@@ -520,20 +530,31 @@ Ikev2AuthPskGenerator (
ChildSaSession->IkeSaSession,
IKEV2_PAYLOAD_TYPE_SA,
IKEV2_CFG_ATTR_INTERNAL_IP6_ADDRESS
);
}
+
+ if (CpPayload == NULL) {
+ goto CheckError;
+ }
+ }
+
+ if (AuthPayload == NULL) {
+ goto CheckError;
}
//
// 4. Generate SA Payload according to the SA Data in ChildSaSession
//
SaPayload = Ikev2GenerateSaPayload (
ChildSaSession->SaData,
IKEV2_PAYLOAD_TYPE_TS_INIT,
IkeSessionTypeChildSa
);
+ if (SaPayload == NULL) {
+ goto CheckError;
+ }
if (IkeSaSession->Spd->Data->ProcessingPolicy->Mode == EfiIPsecTransport) {
//
// Generate Tsi and Tsr.
//
@@ -560,10 +581,13 @@ Ikev2AuthPskGenerator (
IKEV2_NOTIFICATION_USE_TRANSPORT_MODE,
NULL,
NULL,
0
);
+ if (NotifyPayload == NULL) {
+ goto CheckError;
+ }
} else {
//
// Generate Tsr for Tunnel mode.
//
TsiPayload = Ikev2GenerateTsPayload ( @@ -576,10 +600,14 @@
Ikev2AuthPskGenerator (
IKEV2_PAYLOAD_TYPE_NONE,
FALSE
);
}
+ if (TsiPayload == NULL || TsrPayload == NULL) {
+ goto CheckError;
+ }
+
IKE_PACKET_APPEND_PAYLOAD (IkePacket, IdPayload);
IKE_PACKET_APPEND_PAYLOAD (IkePacket, AuthPayload);
if (IkeSaSession->Spd->Data->ProcessingPolicy->Mode == EfiIPsecTunnel) {
IKE_PACKET_APPEND_PAYLOAD (IkePacket, CpPayload);
}
@@ -589,10 +617,45 @@ Ikev2AuthPskGenerator (
if (IkeSaSession->Spd->Data->ProcessingPolicy->Mode == EfiIPsecTransport) {
IKE_PACKET_APPEND_PAYLOAD (IkePacket, NotifyPayload);
}
return IkePacket;
+
+CheckError:
+ if (IkePacket != NULL) {
+ IkePacketFree (IkePacket);
+ }
+
+ if (IdPayload != NULL) {
+ IkePayloadFree (IdPayload);
+ }
+
+ if (AuthPayload != NULL) {
+ IkePayloadFree (AuthPayload);
+ }
+
+ if (CpPayload != NULL) {
+ IkePayloadFree (CpPayload);
+ }
+
+ if (SaPayload != NULL) {
+ IkePayloadFree (SaPayload);
+ }
+
+ if (TsiPayload != NULL) {
+ IkePayloadFree (TsiPayload);
+ }
+
+ if (TsrPayload != NULL) {
+ IkePayloadFree (TsrPayload);
+ }
+
+ if (NotifyPayload != NULL) {
+ IkePayloadFree (NotifyPayload);
+ }
+
+ return NULL;
}
/**
Parses IKE_AUTH packet.
@@ -798,11 +861,15 @@ Ikev2AuthPskParser (
}
//
// 5. Generate keymats for IPsec protocol.
//
- Ikev2GenerateChildSaKeys (ChildSaSession, NULL);
+ Status = Ikev2GenerateChildSaKeys (ChildSaSession, NULL); if
+ (EFI_ERROR (Status)) {
+ return Status;
+ }
+
if (IkeSaSession->SessionCommon.IsInitiator) {
//
// 6. Change the state of IkeSaSession
//
IKEV2_DUMP_STATE (IkeSaSession->SessionCommon.State,
IkeStateIkeSaEstablished); @@ -932,11 +999,17 @@ Ikev2AuthCertGenerator (
}
IkeSaSession = (IKEV2_SA_SESSION *) SaSession;
ChildSaSession = IKEV2_CHILD_SA_SESSION_BY_IKE_SA (GetFirstNode
(&IkeSaSession->ChildSaSessionList));
+ IkePacket = NULL;
+ IdPayload = NULL;
+ AuthPayload = NULL;
CpPayload = NULL;
+ SaPayload = NULL;
+ TsiPayload = NULL;
+ TsrPayload = NULL;
NotifyPayload = NULL;
CertPayload = NULL;
CertReqPayload = NULL;
//
@@ -979,10 +1052,13 @@ Ikev2AuthCertGenerator (
&IkeSaSession->SessionCommon,
IKEV2_PAYLOAD_TYPE_CERT,
(UINT8 *)PcdGetPtr (PcdIpsecUefiCertificate),
PcdGet32 (PcdIpsecUefiCertificateSize)
);
+ if (IdPayload == NULL) {
+ goto CheckError;
+ }
//
// 3. Generate Certificate Payload
//
CertPayload = Ikev2GenerateCertificatePayload ( @@ -991,19 +1067,26 @@
Ikev2AuthCertGenerator (
(UINT8 *)PcdGetPtr (PcdIpsecUefiCertificate),
PcdGet32 (PcdIpsecUefiCertificateSize),
IKEV2_CERT_ENCODEING_X509_CERT_SIGN,
FALSE
);
+ if (CertPayload == NULL) {
+ goto CheckError;
+ }
+
if (IkeSaSession->SessionCommon.IsInitiator) {
CertReqPayload = Ikev2GenerateCertificatePayload (
IkeSaSession,
IKEV2_PAYLOAD_TYPE_AUTH,
(UINT8 *)PcdGetPtr (PcdIpsecUefiCertificate),
PcdGet32 (PcdIpsecUefiCertificateSize),
IKEV2_CERT_ENCODEING_HASH_AND_URL_OF_X509_CERT,
TRUE
);
+ if (CertReqPayload == NULL) {
+ goto CheckError;
+ }
}
//
// 4. Generate Auth Payload
// If it is tunnel mode, should create the configuration payload after the
@@ -1042,20 +1125,31 @@ Ikev2AuthCertGenerator (
ChildSaSession->IkeSaSession,
IKEV2_PAYLOAD_TYPE_SA,
IKEV2_CFG_ATTR_INTERNAL_IP6_ADDRESS
);
}
+
+ if (CpPayload == NULL) {
+ goto CheckError;
+ }
}
+ if (AuthPayload == NULL) {
+ goto CheckError;
+ }
+
//
// 5. Generate SA Payload according to the Sa Data in ChildSaSession
//
SaPayload = Ikev2GenerateSaPayload (
ChildSaSession->SaData,
IKEV2_PAYLOAD_TYPE_TS_INIT,
IkeSessionTypeChildSa
);
+ if (SaPayload == NULL) {
+ goto CheckError;
+ }
if (IkeSaSession->Spd->Data->ProcessingPolicy->Mode == EfiIPsecTransport) {
//
// Generate Tsi and Tsr.
//
@@ -1082,10 +1176,13 @@ Ikev2AuthCertGenerator (
IKEV2_NOTIFICATION_USE_TRANSPORT_MODE,
NULL,
NULL,
0
);
+ if (NotifyPayload == NULL) {
+ goto CheckError;
+ }
} else {
//
// Generate Tsr for Tunnel mode.
//
TsiPayload = Ikev2GenerateTsPayload ( @@ -1098,10 +1195,14 @@
Ikev2AuthCertGenerator (
IKEV2_PAYLOAD_TYPE_NONE,
FALSE
);
}
+ if (TsiPayload == NULL || TsrPayload == NULL) {
+ goto CheckError;
+ }
+
IKE_PACKET_APPEND_PAYLOAD (IkePacket, IdPayload);
IKE_PACKET_APPEND_PAYLOAD (IkePacket, CertPayload);
if (IkeSaSession->SessionCommon.IsInitiator) {
IKE_PACKET_APPEND_PAYLOAD (IkePacket, CertReqPayload);
}
@@ -1115,10 +1216,53 @@ Ikev2AuthCertGenerator (
if (IkeSaSession->Spd->Data->ProcessingPolicy->Mode == EfiIPsecTransport) {
IKE_PACKET_APPEND_PAYLOAD (IkePacket, NotifyPayload);
}
return IkePacket;
+
+CheckError:
+ if (IkePacket != NULL) {
+ IkePacketFree (IkePacket);
+ }
+
+ if (IdPayload != NULL) {
+ IkePayloadFree (IdPayload);
+ }
+
+ if (CertPayload != NULL) {
+ IkePayloadFree (CertPayload);
+ }
+
+ if (CertReqPayload != NULL) {
+ IkePayloadFree (CertReqPayload);
+ }
+
+ if (AuthPayload != NULL) {
+ IkePayloadFree (AuthPayload);
+ }
+
+ if (CpPayload != NULL) {
+ IkePayloadFree (CpPayload);
+ }
+
+ if (SaPayload != NULL) {
+ IkePayloadFree (SaPayload);
+ }
+
+ if (TsiPayload != NULL) {
+ IkePayloadFree (TsiPayload);
+ }
+
+ if (TsrPayload != NULL) {
+ IkePayloadFree (TsrPayload);
+ }
+
+ if (NotifyPayload != NULL) {
+ IkePayloadFree (NotifyPayload);
+ }
+
+ return NULL;
}
/**
Parses IKE_AUTH packet.
@@ -1338,11 +1482,15 @@ Ikev2AuthCertParser (
}
//
// 5. Generat keymats for IPsec protocol.
//
- Ikev2GenerateChildSaKeys (ChildSaSession, NULL);
+ Status = Ikev2GenerateChildSaKeys (ChildSaSession, NULL); if
+ (EFI_ERROR (Status)) {
+ goto Exit;
+ }
+
if (IkeSaSession->SessionCommon.IsInitiator) {
//
// 6. Change the state of IkeSaSession
//
IKEV2_DUMP_STATE (IkeSaSession->SessionCommon.State,
IkeStateIkeSaEstablished); @@ -1539,11 +1687,14 @@ Ikev2GenerateSaKeys (
Status = EFI_SUCCESS;
//
// Generate Gxy
//
- Ikev2GenerateSaDhComputeKey (IkeSaSession->IkeKeys->DhBuffer, KePayload);
+ Status = Ikev2GenerateSaDhComputeKey
+ (IkeSaSession->IkeKeys->DhBuffer, KePayload); if (EFI_ERROR (Status)) {
+ goto Exit;
+ }
//
// Get the key length of Authenticaion, Encryption, PRF, and Integrity.
//
SaParams = IkeSaSession->SessionCommon.SaParams;
@@ -1841,11 +1992,15 @@ Ikev2GenerateChildSaKeys (
if (KePayload != NULL) {
//
// Generate Gxy
//
- Ikev2GenerateSaDhComputeKey (ChildSaSession->DhBuffer, KePayload);
+ Status = Ikev2GenerateSaDhComputeKey (ChildSaSession->DhBuffer, KePayload);
+ if (EFI_ERROR (Status)) {
+ goto Exit;
+ }
+
Fragments[0].Data = ChildSaSession->DhBuffer->GxyBuffer;
Fragments[0].DataSize = ChildSaSession->DhBuffer->GxySize;
}
Fragments[1].Data = ChildSaSession->NiBlock;
--
1.9.5.msysgit.1
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
