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

PVRClient: Do not overwrite arbitrary memory fixes #15157 #15168

Merged
merged 1 commit into from Jan 1, 2019

Conversation

@fritsch
Copy link
Member

commented Dec 31, 2018

strncpy was not properly guarded. Max size in bytes is 64 bytes. Means: 63 chars + \0 speaking of c_str()

Fixes: #15157

@fritsch fritsch requested a review from pkerling Dec 31, 2018

@fritsch fritsch referenced this pull request Dec 31, 2018
2 of 7 tasks complete

@fritsch fritsch force-pushed the fritsch:pvrstring branch from fed8f64 to e44a5d0 Dec 31, 2018

@fritsch

This comment has been minimized.

Copy link
Member Author

commented Dec 31, 2018

Sorry force-pushed: PVR_ADDON_TIMERTYPE_STRING_LENGTH was the right one.

@fritsch

This comment has been minimized.

Copy link
Member Author

commented Dec 31, 2018

jenkins build this please

@fritsch fritsch force-pushed the fritsch:pvrstring branch from e44a5d0 to bf54246 Dec 31, 2018

@fritsch

This comment has been minimized.

Copy link
Member Author

commented Dec 31, 2018

jenkins build this please

@ksooo

This comment has been minimized.

Copy link
Member

commented Dec 31, 2018

Which of the bg_bg strings of pvr.hts is larger than 64 bytes and thus causes the crash?

@ksooo

This comment has been minimized.

Copy link
Member

commented Dec 31, 2018

Okay.

@ksooo

This comment has been minimized.

Copy link
Member

commented Dec 31, 2018

Yeah yeah multibyte chars... PVR C API uses charbuffers everywhere.

@ksooo
ksooo approved these changes Dec 31, 2018
Copy link
Member

left a comment

lgtm

@fritsch

This comment has been minimized.

Copy link
Member Author

commented Dec 31, 2018

  std::string st("Правило на брояча (според справочнка)");
  std::cout << "S: " << st.size() << " " << st << std::endl;

Results in:

S: 68 Правило на брояча (според справочнка) 

See also team channel.

@ksooo

This comment has been minimized.

Copy link
Member

commented Dec 31, 2018

Although the fix is okay - have you guys tested with pvr.hts or did you use another addon?

As far as I can see, pvr.hts cuts the description string and thus no buffer overflow should be possible: https://github.com/kodi-pvr/pvr.hts/blob/master/src/Tvheadend.cpp#L741

@fritsch

This comment has been minimized.

Copy link
Member Author

commented Dec 31, 2018

@ksooo davu tested exactly that. But the description is in kodi's core! Not in the addon.

@fritsch

This comment has been minimized.

Copy link
Member Author

commented Dec 31, 2018

See:

std::string descr(g_localizeStrings.Get(id));
. no cutting at all

@ksooo

This comment has been minimized.

Copy link
Member

commented Dec 31, 2018

@fritsch Ah, now I see the problem. I would fix it like this (shorter than your solution):

strncpy(types_array[i].strDescription, descr.c_str(), sizeof(types_array[i].strDescription) - 1);

PS: I thought it would be a problem with an addon-supplied string which is actually not the case. ;-)

@fritsch

This comment has been minimized.

Copy link
Member Author

commented Dec 31, 2018

I decided against your proposal, cause in that case strncpy will fill the entire strDescription with zeroes when descr.c_str() is not long enough. That's why I included the std::min

@fritsch

This comment has been minimized.

Copy link
Member Author

commented Dec 31, 2018

But: Your code - whatever you want me to put in, I will do :-)

@ksooo

This comment has been minimized.

Copy link
Member

commented Dec 31, 2018

I'm in favor of my proposal because it is shorter and does not introduce additional dependencies, like the define you're using now and std::min.

@fritsch fritsch force-pushed the fritsch:pvrstring branch from bf54246 to 0dba7b9 Dec 31, 2018

@fritsch

This comment has been minimized.

Copy link
Member Author

commented Dec 31, 2018

Updated to your wishes :-)

@fritsch

This comment has been minimized.

Copy link
Member Author

commented Dec 31, 2018

jenkins build this please

@fritsch fritsch merged commit b66ee83 into xbmc:master Jan 1, 2019

1 check passed

default You're awesome. Have a cookie
Details
@ksooo

This comment has been minimized.

Copy link
Member

commented Jan 1, 2019

@fritsch thanks for taking care of this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.