diff mbox series

sae: support default group for H2E

Message ID 20240812171649.163687-1-prestwoj@gmail.com (mailing list archive)
State Accepted, archived
Headers show
Series sae: support default group for H2E | expand

Checks

Context Check Description
tedd_an/pre-ci_am success Success
prestwoj/iwd-alpine-ci-fetch success Fetch PR
prestwoj/iwd-ci-gitlint success GitLint
prestwoj/iwd-ci-fetch success Fetch PR
prestwoj/iwd-alpine-ci-setupell success Prep - Setup ELL
prestwoj/iwd-ci-setupell success Prep - Setup ELL
prestwoj/iwd-ci-makedistcheck success Make Distcheck
prestwoj/iwd-ci-incremental_build success Incremental build not run PASS
prestwoj/iwd-alpine-ci-makedistcheck success Make Distcheck
prestwoj/iwd-alpine-ci-incremental_build success Incremental build not run PASS
prestwoj/iwd-ci-build success Build - Configure
prestwoj/iwd-alpine-ci-build success Build - Configure
prestwoj/iwd-ci-makecheckvalgrind success Make Check w/Valgrind
prestwoj/iwd-ci-clang success clang PASS
prestwoj/iwd-ci-makecheck success Make Check
prestwoj/iwd-alpine-ci-makecheckvalgrind success Make Check w/Valgrind
prestwoj/iwd-alpine-ci-makecheck success Make Check
prestwoj/iwd-ci-testrunner success test-runner PASS

Commit Message

James Prestwood Aug. 12, 2024, 5:16 p.m. UTC
This was seemingly trivial at face value but doing so ended up
pointing out a bug with how group_retry is set when forcing
the default group. Since group_retry is initialized to -1 the
increment in the force_default_group block results in it being
set to zero, which is actually group 20, not 19. This did not
matter for hunt and peck, but H2E actually uses the retry value
to index its pre-generated points which then breaks SAE if
forcing the default group with H2E.

To handle H2E and force_default_group, the group selection
logic will always begin iterating the group array regardless of
SAE type.
---
 src/sae.c | 41 ++++++++++++++++++-----------------------
 1 file changed, 18 insertions(+), 23 deletions(-)

Comments

Denis Kenzior Aug. 12, 2024, 6:14 p.m. UTC | #1
Hi James,

On 8/12/24 12:16 PM, James Prestwood wrote:
> This was seemingly trivial at face value but doing so ended up
> pointing out a bug with how group_retry is set when forcing
> the default group. Since group_retry is initialized to -1 the
> increment in the force_default_group block results in it being
> set to zero, which is actually group 20, not 19. This did not
> matter for hunt and peck, but H2E actually uses the retry value
> to index its pre-generated points which then breaks SAE if
> forcing the default group with H2E.
> 
> To handle H2E and force_default_group, the group selection
> logic will always begin iterating the group array regardless of
> SAE type.
> ---
>   src/sae.c | 41 ++++++++++++++++++-----------------------
>   1 file changed, 18 insertions(+), 23 deletions(-)

Applied.  Do we have test for H2E & force behavior?

Regards,
-Denis
James Prestwood Aug. 12, 2024, 6:30 p.m. UTC | #2
Hi Denis,

On 8/12/24 11:14 AM, Denis Kenzior wrote:
> Hi James,
>
> On 8/12/24 12:16 PM, James Prestwood wrote:
>> This was seemingly trivial at face value but doing so ended up
>> pointing out a bug with how group_retry is set when forcing
>> the default group. Since group_retry is initialized to -1 the
>> increment in the force_default_group block results in it being
>> set to zero, which is actually group 20, not 19. This did not
>> matter for hunt and peck, but H2E actually uses the retry value
>> to index its pre-generated points which then breaks SAE if
>> forcing the default group with H2E.
>>
>> To handle H2E and force_default_group, the group selection
>> logic will always begin iterating the group array regardless of
>> SAE type.
>> ---
>>   src/sae.c | 41 ++++++++++++++++++-----------------------
>>   1 file changed, 18 insertions(+), 23 deletions(-)
>
> Applied.  Do we have test for H2E & force behavior?
Meant to send that, its on the list now.
>
> Regards,
> -Denis
>
diff mbox series

Patch

diff --git a/src/sae.c b/src/sae.c
index 9bce8faa..97c0af05 100644
--- a/src/sae.c
+++ b/src/sae.c
@@ -152,39 +152,34 @@  static int sae_choose_next_group(struct sae_sm *sm)
 {
 	const unsigned int *ecc_groups = l_ecc_supported_ike_groups();
 	bool reset = sm->group_retry >= 0;
+	unsigned int group;
 
-	/*
-	 * If this is a buggy AP in which group negotiation is broken use the
-	 * default group 19 and fail if this is a retry.
-	 */
-	if (sm->sae_type == CRYPTO_SAE_LOOPING && sm->force_default_group) {
-		if (sm->group_retry != -1) {
-			l_warn("Forced default group but was rejected!");
-			return -ENOENT;
-		}
-
-		sae_debug("Forcing default SAE group 19");
+	/* Find the next group in the list */
+	while ((group = ecc_groups[++sm->group_retry])) {
+		/*
+		 * Forcing the default group; only choose group 19. If we have
+		 * already passed 19 (due to a retry) we will exhaust all other
+		 * groups and should fail.
+		 */
+		if (sm->force_default_group && group != 19)
+			continue;
 
-		sm->group_retry++;
-		sm->group = 19;
+		/* Ensure the PT was derived for this group */
+		if (sm->sae_type == CRYPTO_SAE_HASH_TO_ELEMENT &&
+				!sm->handshake->ecc_sae_pts[sm->group_retry])
+			continue;
 
-		goto get_curve;
+		break;
 	}
 
-	do {
-		sm->group_retry++;
+	if (!group)
+		return -ENOENT;
 
-		if (ecc_groups[sm->group_retry] == 0)
-			return -ENOENT;
-	} while (sm->sae_type != CRYPTO_SAE_LOOPING &&
-			!sm->handshake->ecc_sae_pts[sm->group_retry]);
+	sm->group = group;
 
 	if (reset)
 		sae_reset_state(sm);
 
-	sm->group = ecc_groups[sm->group_retry];
-
-get_curve:
 	sae_debug("Using group %u", sm->group);
 
 	sm->curve = l_ecc_curve_from_ike_group(sm->group);