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?
| 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.
2 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.