variable callsigin: length correct? #28
Comments
dl9sau
pushed a commit
to dl9sau/JTEncode
that referenced
this issue
Oct 11, 2021
1. strcpy issues fixed: It's betterr to not trust user input. For some input types, like wspr type2 message callsign, trust may be ok (but even here, invalid input like "DL///CA1LL" or "<CA1LL" could do harm). In the case of "message data", size of input data should be checked before copy. And the case where I had problems: in my program, I have grid locator with more than bytes. I used the locator in other parts of my program, and then I passed it to wspr_encode(). wspr_encode() did a strcpy() fom my input data with len >> 6 bytes to a local variable with size 7. => board rebooted 2. latlon_to_grid function grid result Grid is a string of 6 bytes plus \0. Documentation of the latlon_to_grid function says, user has to provide an array of min 7 bytes. strncpy(ret_grid, grid, 6) copied 6 bytes. grid is 6 bytes. strncpy does not add \0 if length is reached (which is the case here). -> 7 instead off 6 is the fix. Or: just strcpy(). Because both functions cannot assure that the buffer the user provided was correct. 3. char callsign with size 12 should better be 13. See etherkit#28 4. Just a question: In ft_message_prep(), we have char temp_msg[14]; snprintf(temp_msg, 14, "%13s", message); printf "%13s" right-aligns (-> " foo"). Did you mean "%.13s" (fored cut at position 13)? Documentation: diff --git a/src/JTEncode.cpp b/src/JTEncode.cpp index 5a53e37..4f87006 100644 --- a/src/JTEncode.cpp +++ b/src/JTEncode.cpp @@ -67,7 +67,7 @@ void JTEncode::jt65_encode(const char * msg, uint8_t * symbols) { char message[14]; memset(message, 0, 14); - strcpy(message, msg); + strncpy(message, msg, 13); !!!!! ^ strncpy copies 13 bytes. If message is shorter 13 bytes, rest is filled with \0. !!!!! But if souorce msg is >= 13 bytes, strncpy does not null-terminate. !!!!! memset assured, that message[13] is 0. -> We are still null-terminated after this operation // Ensure that the message text conforms to standards // -------------------------------------------------- @@ -111,7 +111,7 @@ void JTEncode::jt9_encode(const char * msg, uint8_t * symbols) { char message[14]; memset(message, 0, 14); - strcpy(message, msg); + strncpy(message, msg, 13); // Ensure that the message text conforms to standards // -------------------------------------------------- @@ -160,7 +160,7 @@ void JTEncode::jt4_encode(const char * msg, uint8_t * symbols) { char message[14]; memset(message, 0, 14); - strcpy(message, msg); + strncpy(message, msg, 13); // Ensure that the message text conforms to standards // -------------------------------------------------- @@ -205,8 +205,8 @@ void JTEncode::wspr_encode(const char * call, const char * loc, const int8_t dbm char call_[13]; char loc_[7]; uint8_t dbm_ = dbm; - strcpy(call_, call); - strcpy(loc_, loc); + snprintf(call_, 13, "%.12s", call); + snprintf(loc_, 7, "%.6s", loc); !!!!!! In this case, you did not memset local variables call_ and loc_. !!!!!! Thus, strncpy(loc_,..., 7) would not terminate if user input is more than 6 bytes. !!!!!! loc_[6] = 0; is one solution. The other ohne is snprintf, here with enforced knoowlege of variable size (7), !!!!!! and with left aligned and cut "%.6s" loc. // Ensure that the message text conforms to standards // -------------------------------------------------- @@ -429,7 +429,7 @@ void JTEncode::ft8_encode(const char * msg, uint8_t * symbols) char message[19]; memset(message, 0, 19); - strcpy(message, msg); + strncpy(message, msg, 18); // Bit packing // ----------- @@ -498,7 +498,7 @@ void JTEncode::latlon_to_grid(float lat, float lon, char* ret_grid) grid[4] = (char)((uint8_t)(lon * 12) + 'a'); grid[5] = (char)((uint8_t)(lat * 24) + 'a'); - strncpy(ret_grid, grid, 6); + strncpy(ret_grid, grid, 7); } /* Private Class Members */ @@ -689,7 +689,7 @@ void JTEncode::wspr_message_prep(char * call, char * loc, int8_t dbm) } call[12] = 0; - strncpy(callsign, call, 12); + strncpy(callsign, call, 13); !!!!! call with length of 12 is a loocal variable. In this part oof the code, call may contain a null-terminated !!!!! string with 12 bytes plus 0. It's copied to global variable callsign with size of 12. !!!!! strncpy() works wel with size 12. But callsign is not null-terminateed. It may not need to. But in the following !!!!! code, tests like strchr(callsign, '...') are made - and these will run out beyond the array. !!!!! I see no harm in enlargeing the size of callsign fromo 12 to 13. // Grid locator validation if(strlen(loc) == 4 || strlen(loc) == 6) diff --git a/src/JTEncode.h b/src/JTEncode.h index 04718a5..9ab45da 100644 --- a/src/JTEncode.h +++ b/src/JTEncode.h @@ -258,7 +258,7 @@ private: uint8_t crc8(const char *); void pad_callsign(char *); void * rs_inst; - char callsign[12]; + char callsign[13]; char locator[7]; int8_t power; };
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
I read through the code because I had an other problem and searched for all occurences of strcpy, strncpy, ..
I see the following problem with callsign, defined in JTEncode.h:
char callsign[12];
We know, compound calls in wspr type2 messages are 12 bytes long: <DL9/DL9SAU>, plus the terminating \0.
JTEncode.cpp does:
strncpy(callsign, call, 12);
-> No buffer overflow, but the copied 12 bytes long string is not terminated.
The operation further down,
char* slash_avail = strchr(callsign, (int)'/');
may search for '/' beyond end of char[] callsign (if user had a wrong input). Same for
char* bracket_avail = strchr(callsign, (int)'>');
Fix:
I see no reason to keep char callsign 12 bytes long.
-> Define in JTEncode.h
char callsign[13];
should solve the hypothetical problem.
The text was updated successfully, but these errors were encountered: