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.

Sign up for GitHub

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

Use r_str_scanf as a safe alternative to fix gdb reg profile parsing bugs #22562

Closed
wants to merge 31 commits into from
Closed

Use r_str_scanf as a safe alternative to fix gdb reg profile parsing bugs #22562

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
7bc0349
Fix conversion from GDB register profile to rizin profile
Crispy-fried-chicken Feb 3, 2024
666c94f
delete some defination as for the reviewer
Crispy-fried-chicken Feb 4, 2024
e9b636b
update for some foramt
Crispy-fried-chicken Feb 4, 2024
d9854e2
fix some bugs for sscanf and update for reviewer's suggestions
Crispy-fried-chicken Feb 6, 2024
8e8a040
fix redeclaration
Crispy-fried-chicken Feb 6, 2024
8ca150d
fix
Crispy-fried-chicken Feb 6, 2024
197a3cc
update for some fix
Crispy-fried-chicken Feb 6, 2024
fa32abb
Merge branch 'master' into patch-2
Crispy-fried-chicken Feb 7, 2024
c3c3ee3
Merge branch 'master' into patch-2
Crispy-fried-chicken Feb 8, 2024
292fa8c
change sscanf to r_str_scanf to fix OOB
Crispy-fried-chicken Feb 8, 2024
b25fd13
fix
Crispy-fried-chicken Feb 8, 2024
ecab0cc
fix
Crispy-fried-chicken Feb 8, 2024
5aa813f
add some check for return value
Crispy-fried-chicken Feb 14, 2024
3003235
Merge branch 'master' into patch-2
Crispy-fried-chicken Feb 14, 2024
e76a620
Apply suggestions from code review
trufae Feb 14, 2024
14c6da7
Apply suggestions from code review
trufae Feb 14, 2024
1853264
fix some lint error
Crispy-fried-chicken Feb 14, 2024
8adfa77
Merge branch 'patch-2' of https://github.com/Crispy-fried-chicken/rad…
Crispy-fried-chicken Feb 14, 2024
a748b70
Merge branch 'master' into patch-2
Crispy-fried-chicken Feb 14, 2024
1cc22fe
Update libr/io/p/io_self.c
trufae Feb 14, 2024
a06f017
Update libr/debug/p/debug_gdb.c
trufae Feb 14, 2024
0e34ceb
Update libr/debug/p/debug_gdb.c
trufae Feb 14, 2024
8793f0f
fix
Crispy-fried-chicken Feb 14, 2024
4cc4769
Apply suggestions from code review
trufae Feb 14, 2024
6dcbd85
Update libr/debug/p/native/linux/linux_coredump.c
trufae Feb 14, 2024
97a28ad
Update libr/debug/p/native/linux/linux_coredump.c
trufae Feb 14, 2024
dbeadf1
Update libr/debug/p/debug_native.c
trufae Feb 14, 2024
c193c46
Update libr/debug/p/debug_native.c
trufae Feb 14, 2024
c3c0906
Apply suggestions from code review
trufae Feb 14, 2024
8698e5a
Update libr/debug/p/native/linux/linux_coredump.c
trufae Feb 14, 2024
d4f2fba
Apply suggestions from code review
trufae Feb 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 3 additions & 7 deletions libr/debug/p/debug_gdb.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ typedef struct {
#define UNSUPPORTED 0
#define SUPPORTED 1


trufae marked this conversation as resolved.
Show resolved Hide resolved
typedef struct plugin_data_t {
RIOGdb ** origriogdb;
libgdbr_t *desc;
Expand Down Expand Up @@ -194,7 +195,7 @@ static RList *r_debug_gdb_map_get(RDebug* dbg) { // TODO
int unk = 0, perm, i;
char *ptr, *pos_1;
size_t line_len;
char name[1024], region1[256], region2[100], perms[5];
char name[1025], region1[101], region2[101], perms[6];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

those extra unaligned bytes will make stack to have holes and wont optimize it properly, also as said those are arbitrary numbers. not tied to gdb definitions

region1[0] = region2[0] = '0';
region1[1] = region2[1] = 'x';
char *save_ptr = NULL;
Expand Down Expand Up @@ -236,12 +237,7 @@ static RList *r_debug_gdb_map_get(RDebug* dbg) { // TODO
r_list_free (words);
free (wordstr);
#else
// We assume Linux target, for now, so -
// 7ffff7dda000-7ffff7dfd000 r-xp 00000000 08:05 265428 /usr/lib/ld-2.25.so
// Android
// "12c00000-12c40000 rw-p 00000000 00:00 0 [anon:dalvik-main space (region space)]";
ret = sscanf (ptr, "%s %s %"PFMT64x" %*s %*s %[^\n]", &region1[2],
perms, &offset, name);
ret = sscanf (ptr, "%" RZ_STR_DEF(98) "s %" RZ_STR_DEF(5) "s %" PFMT64x " %*s %*s %" RZ_STR_DEF(1024) "[^\n]", &region1[2], perms, &offset, name);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dont use those ugly RZ_ things, and dnot hardcode the sizes here, the proper way to use that is to use sizeof(varname) instead of using a define or duplicating the value, this way the final code is much easier to maintain. this is specified in https://wiki.sei.cmu.edu/confluence/display/c if you are curious, also makes the code more readable. But again, ideally i would prefer not to use sscanf, but im fine to merge the fix when complete

#endif
// eprintf ("RET = %d (%s)\n", ret, ptr);
if (ret == 3) {
Expand Down
6 changes: 3 additions & 3 deletions libr/debug/p/debug_io.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,8 @@ static RList *__io_maps(RDebug *dbg) {
}
char *ostr = str;
ut64 map_start, map_end;
char perm[32];
char name[512];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Theres no need to have a define for a size that is tied to an array. Thats defined in the secure coding guidelines as a bad practice because it makes the code more xlobbered and harder to maintain and error probe

char perm[33];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

keep the aligned values, 32 and 512

char name[513];
for (;;) {
char *nl = strchr (str, '\n');
if (nl) {
Expand All @@ -47,7 +47,7 @@ static RList *__io_maps(RDebug *dbg) {
if (_s_) {
memmove (_s_, _s_ + 2, strlen (_s_));
}
sscanf (str, "0x%"PFMT64x" - 0x%"PFMT64x" %s %s",
sscanf (str, "0x%"PFMT64x" - 0x%"PFMT64x" %"RZ_STR_DEF(32)"s %"RZ_STR_DEF(512)"s",
&map_start, &map_end, perm, name);
if (map_end != 0LL) {
RDebugMap *map = r_debug_map_new (name, map_start, map_end, r_str_rwx (perm), 0);
Expand Down
3 changes: 3 additions & 0 deletions libr/include/r_types_base.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -293,4 +293,7 @@ static inline void *untagged_pointer_check(void *x) {
}
#endif

#define RZ_STR_DEF(s) RZ_STR(s)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dont use rizinisms please

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

^ those macros don’t really provide much goodness and their name is from rizin so it should be removed

#define RZ_STR(s) #s

#endif // R2_TYPES_BASE_H
10 changes: 7 additions & 3 deletions libr/reg/profile.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,10 @@
#include <r_reg.h>
#include <r_lib.h>

#define GDB_NAME_SZ 16
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same, those defines are arbitrary, and duplicate the logic below making the code harder to read.

#define GDB_TYPE_SZ 16
#define GDB_GROUPS_SZ 128

static const char *parse_alias(RReg *reg, char **tok, const int n) {
if (n == 2) {
const int role = r_reg_get_name_idx (tok[0] + 1);
Expand Down Expand Up @@ -320,7 +324,7 @@ static char *gdb_to_r2_profile(const char *gdb) {
return NULL;
}
char *ptr1, *gptr, *gptr1;
char name[16], groups[128], type[16];
char name[GDB_NAME_SZ + 1], groups[GDB_GROUPS_SZ + 1], type[GDB_TYPE_SZ + 1];
const int all = 1, gpr = 2, save = 4, restore = 8, float_ = 16,
sse = 32, vector = 64, system = 128, mmx = 256;
int number, rel, offset, size, type_bits, ret;
Expand Down Expand Up @@ -350,8 +354,8 @@ static char *gdb_to_r2_profile(const char *gdb) {
r_strbuf_free (sb);
return false;
}
ret = sscanf (ptr, " %s %d %d %d %d %s %s", name, &number, &rel,
&offset, &size, type, groups);
ret = sscanf(ptr, " %" RZ_STR_DEF(GDB_NAME_SZ) "s %d %d %d %d %" RZ_STR_DEF(GDB_TYPE_SZ) "s %" RZ_STR_DEF(GDB_GROUPS_SZ) "s",
name, &number, &rel, &offset, &size, type, groups);
// Groups is optional, others not
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Groups is optional, others not
// Groups is optional, others are not

if (ret < 6) {
if (*ptr != '*') {
Expand Down
Loading