create S7COMM protocol and tests #1185

wivien19 · 2023-08-21T21:54:24Z

I did not add it to the readme, because I do not know exactly, where to put it.

codecov · 2023-08-21T22:01:44Z

Codecov Report

Merging #1185 (9564367) into dev (7323fb6) will increase coverage by 0.06%.
The diff coverage is 97.41%.

@@            Coverage Diff             @@
##              dev    #1185      +/-   ##
==========================================
+ Coverage   82.66%   82.73%   +0.06%     
==========================================
  Files         157      159       +2     
  Lines       20180    20290     +110     
  Branches     7626     7667      +41     
==========================================
+ Hits        16682    16786     +104     
- Misses       2877     2886       +9     
+ Partials      621      618       -3

Flag	Coverage Δ
alpine317	`72.44% <87.35%> (+0.09%)`	⬆️
centos7	`74.61% <80.24%> (+0.02%)`	⬆️
fedora37	`72.40% <87.35%> (+0.05%)`	⬆️
macos-11	`61.40% <79.38%> (+0.10%)`	⬆️
macos-12	`61.45% <79.38%> (+0.10%)`	⬆️
macos-ventura	`61.43% <80.20%> (+0.11%)`	⬆️
mingw32	`70.28% <83.90%> (+0.02%)`	⬆️
mingw64	`70.34% <83.90%> (+0.07%)`	⬆️
npcap	`83.27% <96.90%> (+0.03%)`	⬆️
ubuntu1804	`75.02% <82.92%> (+0.01%)`	⬆️
ubuntu2004	`73.23% <89.02%> (+0.09%)`	⬆️
ubuntu2204	`72.26% <87.20%> (+0.10%)`	⬆️
ubuntu2204-icpx	`59.29% <74.03%> (+0.05%)`	⬆️
unittest	`82.73% <97.41%> (+0.06%)`	⬆️
windows-2019	`83.31% <96.90%> (+0.06%)`	⬆️
windows-2022	`83.32% <96.90%> (+0.06%)`	⬆️
winpcap	`83.30% <96.90%> (+0.07%)`	⬆️
zstd	`73.85% <94.11%> (+0.12%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
Packet++/header/CotpLayer.h	`90.00% <ø> (ø)`
Packet++/src/CotpLayer.cpp	`97.29% <100.00%> (-0.08%)`	⬇️
Packet++/src/S7CommLayer.cpp	`98.82% <98.82%> (ø)`
Packet++/header/S7CommLayer.h	`92.30% <92.30%> (ø)`

... and 5 files with indirect coverage changes

tigercosmos · 2023-08-22T00:08:13Z

Packet++/src/S7commLayer.cpp

+	}
+
+    std::string S7commLayer::toString() const {
+        std::ostringstream msgTypeStream;


can we have only one ostringstream, there is no need to have four here.

Packet++/header/ProtocolType.h

seladb · 2023-08-25T06:25:16Z

@wivien19 I read a little bit about S7COMM (I didn't know this protocol before), mostly here: http://gmiru.com/article/s7comm/

Apparently this protocol has 3 parts: Header, Parameters and Data.
The protocol implementation in this PR only refers to the Header but doesn't consider the Parameters and the Data. Do you know if it is on purpose?

wivien19 · 2023-08-26T10:17:21Z

I have read the articles on this blog.
I haven not had that purpose yet, because I have not had a lot information of them. I think, with these (parameter and date) can be completed.
Is this header not enough, right?

seladb · 2023-08-27T07:19:23Z

I have read the articles on this blog. I haven not had that purpose yet, because I have not had a lot information of them. I think, with these (parameter and date) can be completed. Is this header not enough, right?

@wivien19 it looks like the Parameter section doesn't have a fixed structure, and so as the Data section. Maybe we can have methods to at least read them as byte arrays?

By the way, even the header structure can change per message:

The header is 10-12 bytes long, the Acknowledgement messages contain two extra error code bytes

wivien19 · 2023-09-05T19:09:05Z

If I want optional parameters (error code), do I need to create another header that I can fit to?

Where should I read the Parameters and the Data values as a byte array? I get its size from fitting it to the header, then I know that it takes up so many bytes. Should I extract a value of variable size from the payload like this?

seladb · 2023-09-06T06:38:57Z

If I want optional parameters (error code), do I need to create another header that I can fit to?

I'm not sure I understand your question - do you mean you'd like to extract the error code from the Parameter data? As far as I know, not all Parameter types have an error code. Some of them have, some don't...

Where should I read the Parameters and the Data values as a byte array? I get its size from fitting it to the header, then I know that it takes up so many bytes. Should I extract a value of variable size from the payload like this?

I think we can have a structure like this, please let me know what you think:

// S7commLayer.h

class S7commLayer : public Layer
{
public:
	...
	...
	class S7CommParameter
	{
		friend class S7commLayer;
	public:
		S7CommParameter(
		uint8_t* getData() { return m_Data; }
		size_t getDataLength() const { return m_DataLen; }
	private:
		S7CommParameter(uint8_t* data, size_t dataLen) : m_Data(data), m_DataLen(dataLen) {}
		uint8_t* m_Data;
		size_t m_DataLen;
	};

	S7CommParameter* getParameter() const;
...
private:
	S7CommParameter* m_Parameter;
};

// S7commLayer.cpp

S7commLayer::getParameter() const
{
	// If m_Parameter is still nullptr - get the parameter data and index and create it
	// m_Parameter = new S7CommParameter(...);
	return m_Parameter;
}

wivien19 · 2023-09-06T08:43:25Z

I think about this error class and error code. These are optional in header and I cannot parse always. I would like to know, that I should create another header to this option or something else. I have no idea to

I think we can have a structure like this, please let me know what you think

I think, it seems to be ok.

seladb · 2023-09-07T04:53:48Z

I think about this error class and error code. These are optional in header and I cannot parse always. I would like to know, that I should create another header to this option or something else. I have no idea to

My understanding based on this: http://gmiru.com/article/s7comm/#21-header is that only Ack-Data messages will have these extra bytes. In that case I think we can create 2 methods inside S7commLayer:

#pragma pack(push, 1)
typedef struct s7comm_ack_data_hdr : s7comm_hdr
{
	uint8_t errorClass;
	uint8_t errorCode;
}
#pragma pack(pop)

class S7commLayer : public Layer
{
public:
	s7comm_hdr* getS7commHeader() const { return (s7commhdr *)m_Data; } // this method already exists
	s7comm_ack_data_hdr *getS7commAckDataHeader() const; // this method will return `nullptr` if the message is not of type `Ack-Data`, otherwise it will return the struct
}

This solution might not be the cleanest, but I think it's good enough since there is only one message type with a different header structure.

Please notice that you need to take the header length into account when implementing the getHeaderLen() method. It should include the total length of the Header, Parameter and Data

wivien19 · 2023-09-22T19:44:42Z

In this commit (c2f1f2c) I have some problem with get parameter values. I have implemented some of the functions, but it does not work good.

Can you help me what i am doing wrong?
I would like to get the value of the parameter and check whether it contains the correct value.

seladb · 2023-09-23T06:29:16Z

In this commit (c2f1f2c) I have some problem with get parameter values. I have implemented some of the functions, but it does not work good.

Can you help me what i am doing wrong? I would like to get the value of the parameter and check whether it contains the correct value.

I think the getParameter() method should be a member of the S7commLayer class, and not of S7CommParameter. We should use it like this:

S7commLayer* myLayer = ...;
S7CommParameter* param = myLayer->getParameter();
...

wivien19 · 2023-09-25T18:19:37Z

I moved the getParameter() method to the other class. It seems to me that I am returning a completely wrong value. How can such a sequence of bytes be returned? I could tell from its size that it was not good, but its value was empty. I tried to change the size, but the value of the data data member was still not correct.

Can you see where I went wrong?

Thank you in advance for your help.

seladb · 2023-09-26T05:54:44Z

I moved the getParameter() method to the other class. It seems to me that I am returning a completely wrong value. How can such a sequence of bytes be returned? I could tell from its size that it was not good, but its value was empty. I tried to change the size, but the value of the data data member was still not correct.

Can you see where I went wrong?

Thank you in advance for your help.

I'm not sure what you mean by wrong size and empty value. Can you please elaborate?
I think you should be able to debug you code, either using a debugger or by adding prints (std::cout) and check the value of variables...

seladb · 2023-10-16T03:26:29Z

@wivien19 is this PR ready for review?

wivien19 · 2023-10-16T08:57:39Z

@wivien19 is this PR ready for review?

Yes

Packet++/header/S7commLayer.h

Packet++/src/S7commLayer.cpp

Packet++/header/S7commLayer.h

seladb · 2023-10-17T07:58:47Z

Packet++/src/S7commLayer.cpp

+	std::string S7commLayer::toString() const
+	{
+		std::ostringstream str;
+		std::string error;
+		if (getMsgType() == 0x03)
+		{
+			error =
+				", error class: " + std::to_string(getErrorClass()) + ", error code: " + std::to_string(getErrorCode());
+		}
+		str << "S7comm Layer, "
+			<< "msg_type: " << std::to_string(getMsgType()) << ", pdu_ref: " << std::to_string(getPduRef())
+			<< ", param_length: " << std::to_string(getParamLength())
+			<< ", data_length: " << std::to_string(getDataLength()) << error;
+
+		return str.str();
+	}


This string is a bit long... maybe it should be shorter, like:

S7Comm Layer, Job Request

What I meant is the string to reflect the message type which can be one of:

@wivien19 I think you misunderstood what I meant. I think the at string should represent the message type. For example:

If the message is of type 0x01 Job Request the string should be: S7Comm Layer, Job Request

If the message is of type 0x02 Ack the string should be: S7Comm Layer, Ack

If the message is of type 0x03 Ack-Data the string should be: S7Comm Layer, Ack-Data

If the message is of type 0x07 Userdata the string should be: S7Comm Layer, Userdata

Else the string could be: S7Comm Layer, Unknown message

seladb · 2023-10-20T05:00:50Z

@wivien19 the Visual Studio build fails in CI: https://github.com/seladb/PcapPlusPlus/actions/runs/6558348298/job/17811739778?pr=1185

Tests/Packet++Test/main.cpp

Tests/Packet++Test/Tests/S7CommTests.cpp

seladb · 2023-10-20T05:28:08Z

Tests/Packet++Test/Tests/S7CommTests.cpp

+	PTF_ASSERT_EQUAL(S7CommLayer->getParamLength(), 12);
+	PTF_ASSERT_EQUAL(S7CommLayer->getDataLength(), 212);
+	PTF_ASSERT_EQUAL(S7CommLayer->getHeaderLen(), 0xea);
+	PTF_ASSERT_EQUAL(S7CommLayer->toString(), "S7Comm Layer, Job Request");


This packet is not of type Job Request, it's Userdata

Tests/Packet++Test/Tests/S7CommTests.cpp

Packet++/src/S7CommLayer.cpp

Tests/Packet++Test/PacketExamples/s7comm_error_code.dat

Tests/Packet++Test/PacketExamples/s7comm_error_code.pcap

Tests/Packet++Test/Tests/S7CommTests.cpp

Packet++/src/S7CommLayer.cpp

seladb · 2023-11-02T02:50:36Z

@wivien19 here are the remaining unresolved comments, can you please address them so we can merge this PR:

wivien19 · 2023-11-03T11:22:32Z

@wivien19 here are the remaining unresolved comments, can you please address them so we can merge this PR:

create S7COMM protocol and tests #1185 (comment)

create S7COMM protocol and tests #1185 (comment)

create S7COMM protocol and tests #1185 (comment)

Sorry, I overlooked them. It was my mistake.

seladb · 2023-11-06T09:24:09Z

Thank you @wivien19 for working on this, much appreciated! 🙏 🙏

create S7COMM protocol and tests

6791f63

wivien19 requested a review from seladb as a code owner August 21, 2023 21:54

add missing override to toString

17c1e4e

wivien19 added 3 commits August 22, 2023 00:08

change null to nullptr

0bdb8d3

systemutils

4482c6a

imports in test

2ba6f6d

tigercosmos reviewed Aug 22, 2023

View reviewed changes

seladb reviewed Aug 22, 2023

View reviewed changes

Packet++/header/ProtocolType.h Outdated Show resolved Hide resolved

wivien19 added 2 commits August 22, 2023 13:31

edit the value of the S7COMM

d85e738

modify toString and add test

81d4434

Merge branch 'dev' into S7COMM

f34c2cf

wivien19 and others added 6 commits September 16, 2023 17:49

modify small changes

46706e9

Merge remote-tracking branch 'origin/S7COMM' into S7COMM

c8d14f8

S7COMM Ack-data header modify and test

d9efe40

Merge branch 'dev' into S7COMM

32c7558

Modify GetHeaderLen

75238d4

Parameter

c2f1f2c

Parameter length

aed4a41

wivien19 force-pushed the S7COMM branch from c366730 to d6a2c9f Compare October 13, 2023 15:18

add error pcap and small modify

4cd7d02

wivien19 force-pushed the S7COMM branch from d6a2c9f to 4cd7d02 Compare October 13, 2023 18:57

seladb reviewed Oct 17, 2023

View reviewed changes

Merge branch 'dev' into S7COMM

0dc3f4d

wivien19 force-pushed the S7COMM branch from c9d4dd6 to 0dc3f4d Compare October 18, 2023 07:35

renaming, remove unneccessary elements

89e04f4

Few fixes

a4a30d5

seladb reviewed Oct 20, 2023

View reviewed changes

seladb and others added 3 commits October 19, 2023 22:47

Few more fixes

8e2636f

Merge branch 'dev' into S7COMM

2f01981

renaming, small fixes

43ecad1

wivien19 force-pushed the S7COMM branch from d73006e to 43ecad1 Compare October 25, 2023 09:19

seladb reviewed Oct 31, 2023

View reviewed changes

seladb and others added 2 commits October 30, 2023 23:05

Remove spaces

f1153d3

renaming, remove unneccessary element

f702af7

modify the header length and the toString method

50640bb

seladb added 2 commits November 6, 2023 00:16

Few more fixes

e738133

Merge branch 'dev' into S7COMM

3972220

seladb approved these changes Nov 6, 2023

View reviewed changes

Add to README

9564367

seladb merged commit f6c8acc into seladb:dev Nov 6, 2023
34 checks passed

seladb mentioned this pull request Nov 8, 2023

Improve fuzzing coverage #1227

Merged

fxlb pushed a commit to fxlb/PcapPlusPlus that referenced this pull request Oct 22, 2024

Add S7COMM protocol (seladb#1185)

b369688

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

create S7COMM protocol and tests #1185

create S7COMM protocol and tests #1185

wivien19 commented Aug 21, 2023

codecov bot commented Aug 21, 2023 •

edited

Loading

tigercosmos Aug 22, 2023

seladb commented Aug 25, 2023

wivien19 commented Aug 26, 2023

seladb commented Aug 27, 2023

wivien19 commented Sep 5, 2023

seladb commented Sep 6, 2023

wivien19 commented Sep 6, 2023

seladb commented Sep 7, 2023 •

edited

Loading

wivien19 commented Sep 22, 2023

seladb commented Sep 23, 2023

wivien19 commented Sep 25, 2023

seladb commented Sep 26, 2023

seladb commented Oct 16, 2023

wivien19 commented Oct 16, 2023

seladb Oct 17, 2023

seladb Oct 20, 2023

seladb Oct 31, 2023

seladb commented Oct 20, 2023

seladb Oct 20, 2023

seladb commented Nov 2, 2023

wivien19 commented Nov 3, 2023

seladb commented Nov 6, 2023

create S7COMM protocol and tests #1185

create S7COMM protocol and tests #1185

Conversation

wivien19 commented Aug 21, 2023

codecov bot commented Aug 21, 2023 • edited Loading

Codecov Report

tigercosmos Aug 22, 2023

Choose a reason for hiding this comment

seladb commented Aug 25, 2023

wivien19 commented Aug 26, 2023

seladb commented Aug 27, 2023

wivien19 commented Sep 5, 2023

seladb commented Sep 6, 2023

wivien19 commented Sep 6, 2023

seladb commented Sep 7, 2023 • edited Loading

wivien19 commented Sep 22, 2023

seladb commented Sep 23, 2023

wivien19 commented Sep 25, 2023

seladb commented Sep 26, 2023

seladb commented Oct 16, 2023

wivien19 commented Oct 16, 2023

seladb Oct 17, 2023

Choose a reason for hiding this comment

seladb Oct 20, 2023

Choose a reason for hiding this comment

seladb Oct 31, 2023

Choose a reason for hiding this comment

seladb commented Oct 20, 2023

seladb Oct 20, 2023

Choose a reason for hiding this comment

seladb commented Nov 2, 2023

wivien19 commented Nov 3, 2023

seladb commented Nov 6, 2023

codecov bot commented Aug 21, 2023 •

edited

Loading

seladb commented Sep 7, 2023 •

edited

Loading