remove GetLastSenderId() API and replace with a RecvMsg() API that returns the AppId of the sender of the message being received #759
Comments
|
A new API as you suggest make sense here as it makes the operation atomic. The sender identifier needs to be unique within a mission and known to the receiver. The app name as a string? The concern Joe brought up is in a distributed system where there may be two apps with the same name on different processors. |
Do we support multiple processors publishing to the same bus? WRT the name, vs. the app id, publishing the name with every packet incurs a significant penalty of having to crosswalk from the ID to the name, even if nobody needs it. See cFE/fsw/cfe-core/src/sb/cfe_sb_api.c Line 1324 in 84ec281 This also resulted in a pointer being returned in the GetLastSenderId() code, and either that or RcvMsgSenderName() would have to be passed in a buffer pointer (and the buffer size and checks to ensure the buffer is valid and long enough.) Inefficient! I suppose there could be a concern that an app would be deleted then a new app created, taking the same AppID. First, if the reader checked the name or some other mechanism on every read, this would only be a problem if an app published a message and exited and a new app took its ID before the message is received (a very small window, hopefully!) A better solution would be to make AppID non-reusable, so I could look up the ID for an app and be assured that any message with that senderid was that app. Currently CFE_ES_CleanupApp() marks the entry as "UNDEFINED", and that logic could be changed to never re-use the entry, but then you run the risk of running out of entries (particularly if you have an app that is restarted on a crash.) For SBN, the problem is ensuring that SBN ignores the message SBN is putting onto the bus. So, of course, the AppID will be valid and easy to check without touching the name, so I have a particular perspective on this. One other option--what if the AppTable entry retains the name even after CleanupApp() is called, and a later call to register the app checks the table for the same name and either rejects the registration or returns the old AppID (and re-sets the table entry)? |
|
Can we split this issue (make this one specific to GetLastSenderId removal)? #764 only covers the removal. |
|
On further review, GetLastSenderId does work as originally intended. From cfe_sb.h "This routine can be used after a successful #CFE_SB_RcvMsg call to find out which application sent the message that was received. ... The *Ptr is valid only until the next call to cFE_SB_RcvMsg for the same pipe" The intent was, an application just read this message from it's pipe and will be executing the command, and is asking who sent it? The AppName string was to be used along with the ProcessorId if needed. SBN preserved (or at least did) the AppName and ProcessorId across the subnetwork(s). The AppName and ProcessorId are globally unique. This API is not really broken, and works as intended. There may be other issues now. Another point in this ticket on avoiding loops, SBN was to use CFE_SB_SubscribeLocal. The effect was that SBN would not receive messages from other processors that SBN had published. SBN's use of GetLastSenderId was to retrieve that information to put in the SBN protocol wrapper around the SB message so that a peer had the original AppName and ProcessorId. I would like to revisit this ticket at a CCB. |
|
But CFE_SB_SubscribeLocal doesn't seem to work that way. It just changes if there is a message sent on subscription (or the send all subscriptions request). SBN should use the IGNOREMINE, which causes it not to get added to the queue. |
|
Also, even if SBN did pass AppName and ProcessorId as part of the SBN protocol wrapper, how would that information get to the app without a way for SBN to tell SB? So you'd be adding a way to spoof (if an app can tell SB where the message came from), which defeats the security rationale. Note a some other header definitions have a "source" field. It is also not a security solution but sounds like it's just as secure as above but less complicated. |
|
You are correct. A few years ago the API was "int32 CFE_SB_SendMsgFull(CFE_SB_Msg_t *MsgPtr, uint32 TlmCntIncrements, uint32 CopyMode, CFE_SB_SenderId_t *SenderPtr)" in SB_priv.h, |
|
We should really clarify what is meant by "security" before using that term. To me "security" means protection against bad actors by design i.e. by using proper crypto + secret keys + secure protocols to ensure that data is authentic cannot be tampered with. But this is generally not possible in an environment where everything shares the same memory space. In CFE, anything can be "spoofed" anywhere if a malicious application wants to do that, regardless if it is a separate API or it just uses a backdoor to change the value. My point is, if an application is being dishonest and wants to pretend it was the authorized sender of a "fire thruster" message, it can do so, and no simple value check will stop it. Something like What I think the intent of this API was is to introduce a second factor into the message checking, such that if something (honestly) sends the message by mistake, it is highly unlikely that BOTH factors would validate, and the message can be dropped. Again, point is to catch an honest mistake, not an intentional forgery. Very different. But that same effect can be had, much more simply without any broken APIs, by designing your messages to have two or more factors built in - instead of having a "fire thruster" command with a single byte, which is easy to create by accident -- use a pair of values or a uint32 where only certain NONZERO values are valid, and/or include a separate "Authentication Key" field inside the same command payload on sensitive commands Apps which might send the message by mistake (maybe sending a buffer of all zeros or all ones, for instance) would be much less likely to accidentally send this command and have the fields check out - even if it is the app which normally sends the command. No need for a dedicated API to accomplish it, just design your command structures accordingly with more than one field that needs to "agree" for the message to be processed. Just simple data structure design solves the problem. Analogy in UI world this is like having an "are you sure?" prompt - intentional commands is easy to send both a command and confirmation, but honest mistakes will lack the confirmation. |
|
FYI I felt it worthwhile, as an exercise, to write an example "pre-shared key" model for SB. Note that the algorithm is a really dumb algorithm that is not secure by any means (in fact it generates a one-byte salt and two bytes of hash using a simple add CRC-like algorithm; 3 bytes total 'cause that's what was left in uint8 Spare[3]; in DestinationD_t. :) ). The thinking is the pre-shared key could be any blob of bytes, but likely would be a string (like "application name" or "your SSN" :D. The hash + salt just makes it fit into a fixed size buffer and would make it a bit more one-way. A proper SHA hash algorithm could be applied, with a significantly larger buffer in/attached to DestinationD_t. Needless to say, for any pipe that subscribes with a PSK, it will not receive messages with that MID unless the sender sends with the same PSK. It's slightly more secure than having a sender ID returned by the RcvMsg() call. I am totally fine if the CCB feels this is the wrong approach. I just felt it interesting to code up the thoughts in my head. I realize this sketch needs docs, unit tests, and likely doesn't even work... |
SBN does use IGNOREMINE now, FYI. Works fine (although the IGNOREMINE code could use a little optimization, I think.) |
I was not clear in this, besides GetLastSenderId() by its very name seems to imply that it gets the last sender not the sender of the message you're reading (which is the first message in the queue.) Anyways...The docs certainly were cross-wise as to what its behavior was as well. |
|
Just to be clear, unless our funding stakeholders explicitly say they want new requirements and/or security development work to be performed, this sort of effort is not covered by the certification resources and likely won't be accepted into the release we are working on for them. It's fine if the open source community wants to branch and work these items, but note that branch would not have any stakeholder resources associated with it. Now if someone comes up with a pot of security improvement resources, that could cover the work (although still likely a separate branch). I don't mean to discourage work, just that we need to focus the resources we do have to meet the stakeholder agreements that are in place. |
…recated fix #759 - deprecates GetLastSenderId()
Describe the bug
The current CFE_SB_GetLastSenderId() API is broken, as indicated in #745 and #744, but also as discussed at today's CCB, the intent and use-case is really "who sent this message" not "who sent the last message" on a pipe. In fact, SBN is currently using this API to prevent SBN message loops, and this API doesn't work correctly for that purpose.
Expected behavior
The suggestion is to remove the GetLastSenderId() API entirely, and add a new receive method like:
int32 CFE_SB_RcvMsgWithSenderId(CFE_SB_MsgPtr_t *BufPtr, uint32 *SenderAppIdPtr, CFE_SB_PipeId_t PipeId, int32 TimeOut);that has an additional out parameter which will contain the AppId of the app that sent the message returned in the BufPtr.Reporter Info
[email protected]
The text was updated successfully, but these errors were encountered: