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

Possible issue: incorrect decoding of DCE/RPC lookup request #555

Open
widavies opened this issue May 26, 2023 · 1 comment
Open

Possible issue: incorrect decoding of DCE/RPC lookup request #555

widavies opened this issue May 26, 2023 · 1 comment

Comments

@widavies
Copy link

I'm currently having an issue where a Proneta scan against my device running this stack returns no IM0 record data.

In other words, this section is missing from Proneta:
image

After investigating this a bit, the particular implementation of pf_cmrdr_inquiry_read_all_reg_ind seems a bit bizarre to me. I'm not super familiar with the Profinet specification, so I might just be missing something here.

The following here is a Wireshark capture of a request made from Proneta to the IO-device. As highlighted here, Proneta chooses a somewhat random sequence number (in this case 370):
image

Taking a look at pf_cmrdr_inquiry_read_all_reg_ind, a switch is performed off of the p_rpc_req->sequence_nmb field:

   switch (p_rpc_req->sequence_nmb)
   {
   case 0:
   {
      // Handle EPM interface
      // ...
   }
   break;
   case 1:
  {
      // Handle PNIO
      // ....

According to the specification, the sequence number is used to uniquely identify an RPC call. From my understanding, the exact value has no further purpose than to distinguish multiple RPC calls.
image

To me it seems like the sequence number should not be checked against the fixed values of 0 and 1. Looking at the packet, I can't determine any other way to handle the lookup requests properly than to keep the p_session_info_t alive instead of killing it after the first request. That way a sequence number can just be handled internally and incremented for the session.

I ending up adding the fix for this in, I'm happy to make a pull request.

typedef struct pf_session_info {
+    uint32_t lookup_sequence_nmb;
}

This sequence number is set to 0 when the session is created and incremented once every time a packet is handled.

static int pf_cmrdr_inquiry_read_all_reg_ind (
   pnet_t * net,
   pf_session_info_t * p_sess,
   const pf_rpc_header_t * p_rpc_req,
   const pf_rpc_lookup_req_t * p_lookup_req,
   pf_rpc_lookup_rsp_t * p_lookup_rsp,
   pnet_result_t * p_read_status)
{
   int ret = -1;

-   switch(p_rpc_req->sequence_nmb)
+   switch(p_sess->lookup_sequence_nmb)
   {
     case 0:
     {
       /* Check if handle is all NULL */
       if(
         memcmp(
           &p_lookup_req->rpc_handle,
           &null_rpc_handle,
           sizeof(null_rpc_handle)) == 0)
       {
         /* Send the EPM interface information */
         LOG_INFO (
           PF_RPC_LOG,
           "EPM(%d): PLC is requesting EPM interface information.\n",
           __LINE__);
         ret = pf_cmrdr_add_epmv4_entry(
           net,
           p_rpc_req,
           p_lookup_req,
           p_lookup_rsp,
           p_read_status);
       }
     }
     break;
     case 1:
     {
       /* Check if handle is NOT NULL */
       if(
         memcmp(
           &p_lookup_req->rpc_handle,
           &null_rpc_handle,
           sizeof(null_rpc_handle)) != 0)
       {
         /* Send the PNIO interface information */
         LOG_INFO (
           PF_RPC_LOG,
           "EPM(%d): PLC is requesting PNIO interface information via EPM.\n",
           __LINE__);
         ret = pf_cmrdr_add_pnio_entry(
           net,
           p_rpc_req,
           p_lookup_req,
           p_lookup_rsp,
           p_read_status);
       }
     }
     break;
     default:
+     p_sess->kill_session = true;
   }

   return ret;
}
// pf_cmrpc_ce_packet

 /* Handle Endpoint Map request */
 ret = pf_cmrpc_lookup_ind (
    net,
    p_sess,
    &rpc_req,
    req_pos,
    max_rsp_len,
    p_sess->out_buffer,
    &p_sess->out_buf_len);
 *p_close_socket = p_sess->from_me;

+ p_sess->lookup_sequence_nmb++;

 /* Close session after each EPM request
    If future more advanced EPM usage is required, implement a
    timeout for closing the session */
- p_sess->kill_session = true;

Per the above comment, this approach also requires a timeout for closing the session. Any ideas on this are welcome.

@pyhys
Copy link
Collaborator

pyhys commented Jun 22, 2023

Hi!
I fully agree that the implementation in pf_cmrdr_inquiry_read_all_reg_ind() is strange, as we check against the sequence number (which will be in the hundreds when running Proneta).
This results in the EPM response lacking any meaningful contents.

However this does not seem to relate to missing content in Proneta. I can not reproduce the issue, as the I&M data is shown every time in Proneta. The I&M0 data is read via a "read implicit" request.

Can you please try to reproduce the missing content issue using the sampleapp from this repo?

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

No branches or pull requests

2 participants