Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

hostapd: 2.9 -> 2.10; wpa_supplicant: 2.9 -> 2.10 #155266

Merged
merged 2 commits into from
Jan 17, 2022
Merged

Conversation

mweinelt
Copy link
Member

Motivation for this change

https://w1.fi/security/2022-1/sae-eap-pwd-side-channel-attack-update-2.txt
https://w1.fi/cgit/hostap/plain/hostapd/ChangeLog
https://w1.fi/cgit/hostap/plain/wpa_supplicant/ChangeLog

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 22.05 Release Notes (or backporting 21.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

name = "CVE-2021-30004.patch";
url = "https://w1.fi/cgit/hostap/patch/?id=a0541334a6394f8237a4393b7372693cd7e96f15";
sha256 = "1gbhlz41x1ar1hppnb76pqxj6vimiypy7c4kq6h658637s4am3xg";
url = "https://raw.githubusercontent.com/openwrt/openwrt/eefed841b05c3cd4c65a78b50ce0934d879e6acf/package/network/services/hostapd/patches/300-noscan.patch";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a particular reason for this URL change? The pointed content seems identical.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess explicitly pointing to a specific commit pins a bit further the content, sounds good to me.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it was scratching an itch.

Copy link
Member

@picnoir picnoir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Manually tested this on a couple of deployments.

LGTM

@picnoir picnoir merged commit 9739b15 into NixOS:master Jan 17, 2022
@mweinelt mweinelt deleted the hostapd branch January 17, 2022 13:54
Comment on lines 17 to 18
# Note: fetchurl seems to be unhappy with openwrt git
# server's URLs containing semicolons. Using the github mirror instead.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Since we are not using the github link we could have also used fetchpatch.

@ncfavier
Copy link
Member

ncfavier commented Feb 7, 2022

Am I the only one for whom this completely broke wpa_gui? (The program starts but the window is never drawn)

I bisected it to upstream commit eadfeb0e93748eb396ae62012b92d21a7f533646 (you can clone the repo from http://w1.fi/hostap.git), looking into what's wrong now.

@vcunat
Copy link
Member

vcunat commented Feb 7, 2022

For me it shows the window well, but my machine does not have wpa_supplicant or WiFi.

@ncfavier
Copy link
Member

ncfavier commented Feb 7, 2022

Ok so the problem is that my list of networks (wpa_cli list_networks) looks like

0   mutable0
1   mutable1
2   mutable2
...
0   immutable0
1   immutable1

(I am using networking.wireless.allowAuxiliaryImperativeNetworks)

and the logic in that commit assumes that the IDs are strictly increasing, otherwise it might get stuck in an infinite loop.

The fact that the immutable network IDs start again from 0 is probably a bug in our 0001-Implement-read-only-mode-for-ssids.patch, cc @Ma27 EDIT: never mind, it's because -c and -I both result in a call to wpa_config_read, which starts numbering at 0. Definitely an upstream bug.

Fixing this could be as simple as declaring the id and cred_id variables in wpa_config_read as static.

@Ma27
Copy link
Member

Ma27 commented Feb 7, 2022

Definitely an upstream bug.

Well, we (to be precise, I) messed around in their code, so not sure if that still counts as upstream bug :)

I'll take a look.

@ncfavier
Copy link
Member

ncfavier commented Feb 7, 2022

I mean that it would still happen without your patch.

@ncfavier
Copy link
Member

ncfavier commented Feb 7, 2022

Using the patch suggested above solves the issue:

--- a/wpa_supplicant/config_file.c
+++ b/wpa_supplicant/config_file.c
@@ -297,8 +297,8 @@ struct wpa_config * wpa_config_read(const char *name, struct wpa_config *cfgp)
 	struct wpa_ssid *ssid, *tail, *head;
 	struct wpa_cred *cred, *cred_tail, *cred_head;
 	struct wpa_config *config;
-	int id = 0;
-	int cred_id = 0;
+	static int id = 0;
+	static int cred_id = 0;

 	if (name == NULL)
 		return NULL;

@Ma27
Copy link
Member

Ma27 commented Feb 7, 2022

Seems reasonable to me. Do you want to file a PR? %)

@ncfavier
Copy link
Member

ncfavier commented Feb 7, 2022

Yes, I'll send the patch to their mailing list.

@Ma27
Copy link
Member

Ma27 commented Feb 7, 2022

I'd say it's OK to file a PR here as well :)

@ncfavier
Copy link
Member

ncfavier commented Feb 7, 2022

Oh, yes, will do that as well.

ncfavier added a commit to ncfavier/nixpkgs that referenced this pull request Feb 7, 2022
@ncfavier
Copy link
Member

ncfavier commented Feb 7, 2022

Opened #158505 and sent the patch upstream

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants