Fix roaming falsepos#1014
Conversation
untitaker
left a comment
untitaker
left a comment
There was a problem hiding this comment.
So I wonder: The firmware on my tplink is able to detect roaming with (afaik) 100% accuracy. how does it do that? can we use that same logic, and how much refactoring would we need?
It also seems like you'd want to persist likely_* variables across recordings, at least. That refactor should be feasible short-term: We change the recording code to cache and reuse analyzer instances across recordings, the analyzer interface gets a fn reset(&mut self) { *self = Default::default() } that we override in this analyzer to not reset those variables.
| } | ||
|
|
||
| // Sometimes an ENB can have multiple PLMNS | ||
| fn format_plmn_list(&mut self, plmn_list: &PLMN_IdentityList) -> String { |
There was a problem hiding this comment.
these functions only need &self
| event_type: EventType::High, | ||
| message: "Disconnected after Identity Request without Auth Accept".to_string(), | ||
| }); | ||
| if self.likely_enb_plmn == self.likely_ue_plmn { |
There was a problem hiding this comment.
I think you basically already said this in the PR description but: If I restart recording and the first thing the analyzer sees is disconnect or identity request, both of these fields will be unknown and the analyzer will just assume we're on the home network.
I'm not saying to change the severity for that case but maybe it could get its own message?
There was a problem hiding this comment.
this continues the thread untitaker started at https://github.com/EFForg/rayhunter/pull/1014/changes#r3176744454
if self.likely_enb_plmn and self.likely_ue_plmn are both "Unknown", i think we should flag a low priority event here. do you agree @cooperq?
There was a problem hiding this comment.
yea that seems reasonable to me. I think we should also cache the likely_ue_plmn when we get it, maybe in config?
| format!("{}-{}", mcc_digits, mnc_digits) | ||
| } | ||
|
|
||
| fn plmn_vec_to_str(&mut self, bytes: &[u8]) -> String { |
There was a problem hiding this comment.
ignoring what the spec says, it seems deku as used inside of pycrate will guarantee that bytes.len() >= 3. but this is pretty far away from this code. can we add a code comment as to why we think this won't ever panic?
and/or add some sort of integration test for this?
| timeout_counter: 0, | ||
| flag: None, | ||
| // You will likely wonder why this isn't an Option<PLMN{mcc: u32, mnc: u32}> | ||
| // The answer is that I like strings. |
There was a problem hiding this comment.
i'm just saying, AI is very good at cleaning up the typing situation without too much effort.
| if reject.emm_cause.inner | ||
| == AttachRejectEMMCause::EPSServicesAndNonEPSServicesNotAllowed | ||
| { | ||
| self.flag = Some(Event { |
There was a problem hiding this comment.
isn't this duplicating what we have in fn transition?
| ie: &InformationElement, | ||
| packet_num: usize, | ||
| ) -> Option<Event> { | ||
| // Set the enodeb plmn to the last sib1 we got, we should improve this once we have PCI data, this |
There was a problem hiding this comment.
Reporting from my austrian captures. In our meeting I had the concern that all the various MVNOs in Austria have their own MNC and would inherently trigger roaming conditions, but this is only partially true, and not too different from the US situation (AT&T seems to have dozens of different MNCs)
I found something in my old captures though: On tplink it appears that we are getting InformationElements from all kinds of cells, including neighboring ones that we aren't attached to at all, so likely_enb_plmn oscillates between a lot of bullshit values.
At the same time likely_ue_plmn is always Unknown and never overwritten, because old_tai is always None.
So this heuristic always self-disables in those captures.
I still feel that something about this heuristic is architecturally unsound. If the user can just hit "restart recording" to reset the state machine, the amount of false positives and false negatives they can trigger with that is unbounded and can't be reasoned about.
In the meeting, @wgreenberg mentioned that information about the home network can be extracted from the orbic's filesystem, maybe we should just read that file and deal with the device portability issues this causes. We could fall back to the "home network detection" that exists in this PR if a device-specific mechanism isn't available.
There was a problem hiding this comment.
On tplink it appears that we are getting
InformationElementsfrom all kinds of cells, including neighboring ones that we aren't attached to at all, solikely_enb_plmnoscillates between a lot of bullshit values.
this is true, however, if you apply this patch and run rayhunter-check against your captures, you'll see that every transition happens with likely_enb_plmn "262-01" so while we are seeing other values here likely_enb_plmn seems consistent when it matters
At the same time
likely_ue_plmnis alwaysUnknownand never overwritten, becauseold_taiis alwaysNone.So this heuristic always self-disables in those captures.
to to clear/nitpicky, this PR never disables the heuristic and instead just reduces the priority of its messages which personally makes me feel a lot more comfortable about any changes here
as for likely_ue_plmn, my somewhat shaky understanding is old_tai will only be set after the device has successfully attached to a tower at least once. i'm basing this on links like these. if you didn't have an active SIM in your device for the area you were in, this is exactly what i'd expect
when running the code in this PR against the captures sent to EFF, likely_ue_plmn does actually get set in many cases
I still feel that something about this heuristic is architecturally unsound. If the user can just hit "restart recording" to reset the state machine, the amount of false positives and false negatives they can trigger with that is unbounded and can't be reasoned about.
isn't part of this just the inherent nature of heuristics that rely on multiple messages? if the recording misses a relevant part of the traffic, it won't have all the context it needs to flag the recording. my hope is with the change i suggested here, we can avoid at least most false positives and we can iterate on this over time based on the submissions we continue to get
afeda38 to
ac1be4d
Compare
bmw
left a comment
bmw
left a comment
There was a problem hiding this comment.
I get the presumed PLMN for the ENB from the latest sib1 packet. This could be a problem if sibs come out of order. I could check the earfcn to solve this but I think checkign the PCI would be better, so once will lands that we can improve this.
is this something you think we really should try and tackle in the first version of this?
if so, i could use some elaboration on what you had in mind here. my first thought is you were essentially wanting to make likely_enb_plmn a map from earfcn/pci values to plmns, but for that to work i think we'd also need to get the earfcn/pci from the NAS messages before calling self.transition and i don't see how to do that
any tips/pointers (especially to the message types i should be looking at) or should i just not worry about this piece for now?
| if self.likely_ue_plmn == "Unknown" { | ||
| self.likely_ue_plmn = self.extract_plmn(&request.old_tai.inner); | ||
| } |
There was a problem hiding this comment.
do we really only want to set likely_ue_plmn once or if we see another old_tai value that actually contains a PLMN, should we update it?
the latter makes more sense to me but i may be misunderstanding things
There was a problem hiding this comment.
I think it's fine to update the likely_ue_plmn
| event_type: EventType::High, | ||
| message: "Disconnected after Identity Request without Auth Accept".to_string(), | ||
| }); | ||
| if self.likely_enb_plmn == self.likely_ue_plmn { |
There was a problem hiding this comment.
this continues the thread untitaker started at https://github.com/EFForg/rayhunter/pull/1014/changes#r3176744454
if self.likely_enb_plmn and self.likely_ue_plmn are both "Unknown", i think we should flag a low priority event here. do you agree @cooperq?
hmmm we could ignore it for now and if its an issue we can work on it later. |
my understanding is it's pretty complicated. i think this might be something we want to work towards in the long term, but i think at least cooper and i would prefer not to block the initial version of this PR on adding this feature additionally, one thing i personally like about the approach here is i believe
cooper and i talked about this more after his comment here and i managed to convince him it's a bad idea for now. my biggest concern is that we'd get different results running we could and maybe should also start exporting rayhunter analyzer state as part of captures, but that's a pretty major overhaul and yet another one i'd like to not block this PR on what do you think? does this and my comment here assuage your high level design concerns especially since again this PR only changes flagged event levels rather than adding or removing any of them or do you strongly feel we should take a different approach here? |
ok 👍
I think this is a good point and that changes my position a bit on what should be in-scope for this PR. it seems like stateful analyzers really are too much effort for now.
to me it doesn't make a ton of difference, things that used to be red are now green, so they won't be submitted to the EFF. we're generally not set up well to detect false negatives from user reports. if we ship this and it turns out 99% of events now have demoted severity due to a bug, there's nothing flagging this
[after 5 stealth edits] yeah you're right.. i was under the impression that most of these multi-message paths are gated by timeout (so that the state eventually clears and doesn't linger for hours), but that doesn't seem to be the case. so i guess we already have that problem. |
3 participants
Pull Request Checklist
cargo fmt.You must check one of:
I used an AI to assist me in writing plmn_vec_to_str and plmn_identity_to_str because I couldn't figure out the rust syntax but the logic behind it was figured out manually by me. I understand every line of code in those functions and feel confident about it. Feel free to suggest rewrites if the AI leaves a bad taste in your mouth, but the bit twiddling in plmn_vec_to_str is IMO the best way to do this, and I tested it extensively. Every other LOC in this PR was artisinally hand crafted by me
The issue this solves is that we get a lot of false positives which are mostly due to roaming errors, the user equipment roams onto an enodeb not belonging to its network. the network logically asks for the identity of the phone and then logically rejects it. These situations should mostly be considered false positives IMO, I'm much more interested when your home PLMN does this. (though its possible that an IMSI catcher could do this and be indistinguishable from a false positive I think that one would ideally want an IMSI catcher to operate on all of the major PLMNs to increase the chances of a phone camping on your network)
This is a draft because I want to give it some thought before we publish this, my biggest concern is that we could start to get false negatives because of this, but I also don't want to continue to scare users with false positives. I think putting false positives as low severity hits that balance but I'm open to other ideas.
I know that @wgreenberg is going to disapprove of my use of strings instead of an Option containing a PLMN struct {mcc: u16, mnc: u16}. I have no justification for this other than that I like strings, and it should probably be fixed. This would also solve the issue we currently have where I am naively comparing two strings one of which may have multiple PLMN values. But right now I was just trying to get it good enough to make my analysis jobs easier.
Two other potential issues are the naive way I'm getting PLMNs. I get the presumed PLMN for the ENB from the latest sib1 packet. This could be a problem if sibs come out of order. I could check the earfcn to solve this but I think checkign the PCI would be better, so once will lands that we can improve this. To get the PLMN of the UE I'm getting the PLMN from the phones previous tracking area the first time we see it. This seems to be the only place I can reliably find the PLMN other than in the IMSI itself, and I don't actually have a good way to do that right now! So far this seems accurate but I guess could end up generating false positives if the phone roamed onto another network before the start of this analysis. I'm not really sure how to make this more robust.
The final issue is that I've had to comment out the state transition from a RRCConnectionRelease. Because if I leave that there it alerts once for that packet and then alerts again at low severity for the following AttachReject message if its the false positive EPSServicesAndNonEPSServicesNotAllowed reason. Since an RRCConectionRelease should always be followed or preceded by an AttachReject messgae I think this is okay for now but I would like to find a cleaner way to do it.